Port variable-length-quantity exercise. #960

Javran · 2021-02-27T06:41:59Z

Closes #959.

Few questions that I'll need help from maintainer:

What is the version number of an exercise? I couldn't find any from problem spec repo.
What should I put in config.json, I'm not sure about pretty much all fields in there.
What do you use for formatting automation? Space-padding stuff and explicit import list is pretty much everywhere nowaways but I find those practices counterproductive so unless there's an automation I'll leave my own formatting style as is.

sshine

Hi, and thanks for the PR! As I mentioned in #955, I'm going to wait with merging this until CI has been unbroken in #958. In the meantime, here is some feedback. Also, thanks for all the good questions. They suggest that there are places where we can improve track documentation.

Position in config.json: The order which configlet (the Exercism tool) was changed in #936 and was summarized in this comment. This should be moved to the track documentation, too.

I apologize for the messy PR review. Normally I would provide a short, concrete list of things that must be changed, but this is the first exercise that is ported after Exercism migrated to v3, and the Haskell track has not been properly migrated yet.

I'll create some issues that address the particular roadblocks that are not solvable within this PR, and then I'll get back to this PR. In the meantime, feel free to address / answer the points I've highlighted.

config.json

exercises/.gitignore

sshine · 2021-02-28T12:02:03Z

exercises/practice/variable-length-quantity/.meta/tests.toml

+
+# multiple values
+"a91e6f5a-c64a-48e3-8a75-ce1a81e0ebee" = true
+


It seems that you are more informed than me about the purpose of .meta/tests.toml.

I'd have to get back to you about this, or perhaps you can link to where the format and purpose reads.

Actually I don't either, but many other exercise has this file and its content seems straightforward to follow.

sshine · 2021-02-28T12:31:08Z

exercises/practice/variable-length-quantity/examples/success-standard/src/Vlq.hs

+{-# LANGUAGE LambdaCase #-}
+{-# LANGUAGE NumericUnderscores #-}
+
+module Vlq


How about VariableLengthQuality?

Suggested change

module Vlq

module VariableLengthQuality

I have the impression that one word module name seems to be preferred on exercises that have long names, but I couldn't find a good one, Encode is probably fine, given that Data.Text.Encoding in text does have both encoding and decoding functions.

sshine · 2021-02-28T12:31:58Z

exercises/practice/variable-length-quantity/examples/success-standard/src/Vlq.hs

+encodes :: [Word32] -> [Word8]
+encodes = concatMap encodeOne
+
+decodeOne :: MonadError DecodeError m => [Word8] -> m Word32


Very nice to have something like this in an example.

Thanks! Something needs to be addressed in this part?

No, it's perfectly fine.

sshine · 2021-02-28T12:36:03Z

exercises/practice/variable-length-quantity/examples/success-standard/package.yaml

@@ -0,0 +1,22 @@
+name: variable-length-quantity
+version: 0.0.0.1 # TODO: what should this number be?


Example solutions don't have a version number.

sshine · 2021-02-28T12:36:10Z

exercises/practice/variable-length-quantity/package.yaml

@@ -0,0 +1,23 @@
+name: variable-length-quantity
+version: 0.0.0.1 # TODO: what should this number be?


This question cannot currently be answered.

This track's exercise versioning policy is described in the track README:

https://github.com/exercism/haskell#exercise-versioning

Which originally meant "Go to the exercise's canonical-data.json and look up the version property". But since exercism/problem-specifications#1674, canonical data no longer contains exercise versions. The reason is political and has to do with people disagreeing about what should go into the problem-specifications repository. Some people got upset, the repository was frozen for a year, and the aftermath is that versioning was removed.

Some of the technical argumentation refers to automated test generators:

To prevent breaking changes, the canonical data is currently versioned using SemVer, so in theory test generators could use this to do "safe" updates. In practice though, most test generators always use the latest version to generate their exercises.

Since the Haskell track does not employ an automated test generator, the test generator is a person who does use the latest version, but does so manually. This reasoning does not seem to apply to this track as long as we manually maintain test files.

[...]

There is no longer any discussion whether a change is a patch, minor or major update.

We no longer need the versioning of the canonical data.

tl;dr: Canonical versions were removed, and our exercise versioning policy depends on them, so we need to make a new versioning policy.

sshine · 2021-02-28T12:57:17Z

exercises/practice/variable-length-quantity/test/Tests.hs

+  { uuid :: T.Text
+  , description :: T.Text
+  , inputAndExpected :: CaseInputExpect ty
+  }


Is this test suite made better with the use of type families? I'm a little unsure if the point to use them here is because the author finds them natural (in which case one may consider if the novelty budget was exceeded for the average student), or if there is a didactic opportunity here?

I'm probably having too much fun porting exercise tests from problem spec. But then I feel all those uses are simple cases that can serve teaching purposes: promoted datatype to tag Case so encode test cases and decode test cases can have records whose types are different. And a bit of type level numbers that, in hindsight, is probably a contrived way of saying "pretty printing width depends on type" (I could just implement instance Show (PrettyHex a) for Word8 ~ a and Word32 ~ a, but we can get into overlapping instances once we start talking about what if now we want Show a => Show (PrettyHex a), which is definitely beyond scope for just teaching Haskell)

Let me know if adding more comments in code to explain those language features could mitigate your concerns or I should avoid using them all together.

Your comment also raises a question that I think could be nice to be addressed more explicitly: are students suppose to treat source code of tests as a blackbox or they are welcomed to read and learn from them, when I wrote those tests I thought it was the former but it now looks more like the latter.

sshine · 2021-02-28T12:57:59Z

exercises/practice/variable-length-quantity/test/Tests.hs

+        forM_ testCases $
+          \( Case
+               { description
+               , inputAndExpected = (input, expected)


This appears to be a mash-up between named and anonymous product types.

Why is inputAndExpected not two separate record fields?

Ah I'm just bundling them together to avoid using two type families (originally CaseInputExpect was separated as CaseInput and CaseExpected)

sshine · 2021-02-28T13:01:44Z

exercises/practice/variable-length-quantity/test/Tests.hs

+        let PrettyHex encoded = encodes' [x]
+            mask = 0b1000_0000
+         in all ((/= 0) . (Bits..&. mask)) (init encoded)
+              .&&. ((last encoded Bits..&. mask) == 0)


I wonder if unqualified imports of these operators will be more easily understood.

I.e. .&. instead of Bits..&. because of the operator beginning with ..

This has to do with QuickCheck also exporting .&., I could unqualify it but it'll still raise the question of which .&. is this one, but arguably this is straightforward to figure out from context.

Turns out this simply needs to be unique to be used in the database and website URLs. Co-authored-by: Simon Shine <[email protected]>

This is not in the scope of this PR, probably do another one. Co-authored-by: Simon Shine <[email protected]>

Javran · 2021-02-28T20:52:08Z

Thanks for your detailed feedback! I've addressed ~~all~~ most of them, please take a look.

I'd also love to be more consistent in terms of code formatting if there is a preferred formatting automation.

Plus this does look nicer.

sshine · 2021-03-04T08:48:46Z

I'll get back to this in the weekend! :-)

Port variable-length-quantity exercise.

ea57dcc

iHiD requested a review from ErikSchierboom February 27, 2021 14:33

sshine suggested changes Feb 28, 2021

View reviewed changes

Javran and others added 2 commits February 28, 2021 11:56

Attach an UUID to the exercise

f6b4e0a

Turns out this simply needs to be unique to be used in the database and website URLs. Co-authored-by: Simon Shine <[email protected]>

Remove exercises/.gitignore

8ed70ad

This is not in the scope of this PR, probably do another one. Co-authored-by: Simon Shine <[email protected]>

Give recursive function a name.

2ffd9bd

Plus this does look nicer.

sshine added the new exercise label Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port variable-length-quantity exercise. #960

Port variable-length-quantity exercise. #960

Javran commented Feb 27, 2021

sshine left a comment •

edited

Loading

sshine Feb 28, 2021

Javran Feb 28, 2021

sshine Feb 28, 2021

Javran Feb 28, 2021

sshine Feb 28, 2021

Javran Feb 28, 2021

sshine Mar 1, 2021

sshine Feb 28, 2021

sshine Feb 28, 2021 •

edited

Loading

sshine Feb 28, 2021

Javran Feb 28, 2021

sshine Feb 28, 2021

Javran Feb 28, 2021

sshine Feb 28, 2021

Javran Feb 28, 2021

Javran commented Feb 28, 2021 •

edited

Loading

sshine commented Mar 4, 2021


		# multiple values
		"a91e6f5a-c64a-48e3-8a75-ce1a81e0ebee" = true

		@@ -0,0 +1,22 @@
		name: variable-length-quantity
		version: 0.0.0.1 # TODO: what should this number be?

		@@ -0,0 +1,23 @@
		name: variable-length-quantity
		version: 0.0.0.1 # TODO: what should this number be?

Port variable-length-quantity exercise. #960

Are you sure you want to change the base?

Port variable-length-quantity exercise. #960

Conversation

Javran commented Feb 27, 2021

sshine left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshine Feb 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Javran commented Feb 28, 2021 • edited Loading

sshine commented Mar 4, 2021

sshine left a comment •

edited

Loading

sshine Feb 28, 2021 •

edited

Loading

Javran commented Feb 28, 2021 •

edited

Loading