new exercise to encode/decode variable length quantity #322

mattetti · 2016-04-28T03:01:19Z

I'm not sure if that shouldn't move this exercise as a problem. I feel that the problem should be more defined so people understand the goal of the exercise.

mattetti · 2016-04-28T03:13:01Z

btw, I'm confused about the failing tests, they are supposed to, that's the whole point of the exercise, isn't it?

…l be moved to the problem

IanWhitney · 2016-04-29T00:40:47Z

Your example file needs to provide a working solution. Travis uses it to make sure your tests are valid. Students won't see your example, it's only used as part of the CI process.

kytrinyx · 2016-04-29T02:13:13Z

exercises/variable-length-quantity/example.go

@@ -0,0 +1,15 @@
+package variablelengthquantity


The example.go should be a reference solution—something that passes the given test suite. It doesn't get delivered when people fetch the exercise, it's just used to make sure that the test suite makes sense.

Oh. Hah. I see that Ian said the same thing below.

mattetti · 2016-04-29T05:47:06Z

Oops sorry, I missed that when I read the guidelines. Just for my own sake, will the users be provided with an empty implementation file?

PR update coming

petertseng · 2016-04-29T06:20:34Z

will the users be provided with an empty implementation file?

They will not, unless such a file is explicitly provided. The rule is any file with example in its name doesn't get shown to students, and all other files do.

For example leap/leap.go is given to students, and leap/example.go isn't.

As for whether to provide a stub file for this exercise, we discussed in #279 and can revisit, but the general feeling back then was that by the time of reaching the later exercises it isn't really needed anymore.

mattetti · 2016-04-29T06:31:21Z

PR updated, tests are now passing. I listed this exercise as the last one because I wasn't sure where it would fit, fell free to move it around.

I respect your decision in regards to #279 even tho I disagree. At the end of the day it's matter of perspective, type of exercises and what part of the code you'd want students to focus on. It's all good with me.

petertseng · 2016-04-29T06:32:46Z

exercises/variable-length-quantity/variable_length_quantity_test.go

+
+	for i, tc := range testCases {
+		t.Logf("test case %d - %#v\n", i, tc.input)
+		if o, _ := DecodeVarint(tc.input); o != tc.output {


Hmm, given that the second return value is being discarded, it could be anything.

Given that in the example implementation it's the number of bytes consumed, now I want to ponder a few things.

These tests expect a DecodeVarint that decodes a single number it finds in a buffer.

Should we specify (and therefore test) DecodeVarint's behavior:

If there are multiple numbers in the buffer? []byte{0x00, 0x00}

If there is not a complete number in the buffer? []byte{0x80, 0x80}

If the buffer is empty? (which may be a subcase of the "not a complete number in the buffer" case)

Also, as an extension of this funciton that decodes a single varint, should we ask for an implementation of a function that decodes as many numbers as possible from a buffer as well? It would expand the scope of the exercise, though!

kytrinyx · 2016-10-01T15:35:23Z

I just realized that we never got this merged. @mattetti would you have a moment to look it over and decide if you're happy with it? @petertseng had a few thoughts, but they were more in the "let's ponder this interesting question" than "we need to change this". We could ship it and then see how the exercise does in the wild.

What do you think?

mattetti · 2016-10-01T16:00:00Z

It's been a while indeed! I just went through the exercise and re-read @petertseng's comments. I agree that his suggestions would be nice to add but aren't blockers.

@kytrinyx I think it's ready to be merged with one caveat, I'm not sure that the goal of the exercise might not be clear without more context. However since I'm not a exercism user I am not sure what the expectations are or even where to better describe the goal of the exercise and give more context.

petertseng · 2016-10-01T17:06:15Z

I've reread my comments too.

I do believe that "no we don't need to, so no changes are necessary" are valid answers to my questions of the form "should we specify these behaviours? / add this other function?"

One thing I would like to make sure on - given that the second return value of DecodeVarint is never checked by the tests - to me this says it should just be removed. Otherwise students have to stick a dummy value in there, to no benefit to them. (If wanting to keep it, the test should check it)

I'm not sure that the goal of the exercise might not be clear without more context.

We have this in https://github.com/exercism/x-common/blob/master/exercises/variable-length-quantity/description.md - I know in exercism/problem-specifications#239 there was uncertainty too on whether it was clear. To me, on reading the current description I don't see any specific place where I would say it was unclear, so I think we are good on that front! (But if anyone disagree, that is the place to send the PRs to make it clearer!)

kytrinyx · 2016-10-01T21:37:26Z

One thing I would like to make sure on - given that the second return value of DecodeVarint is never checked by the tests - to me this says it should just be removed. Otherwise students have to stick a dummy value in there, to no benefit to them. (If wanting to keep it, the test should check it)

That's a good point. What do you think @mattetti?

mattetti · 2016-10-02T00:01:25Z

The API is consistent with the std lib in this case. I would suggest to
update the tests to check the value instead of removing it.
Unfortunately I don't currently have time to do so myself at the moment.

On Sat, Oct 1, 2016, 14:37 Katrina Owen [email protected] wrote:

One thing I would like to make sure on - given that the second return
value of DecodeVarint is never checked by the tests - to me this says it
should just be removed. Otherwise students have to stick a dummy value in
there, to no benefit to them. (If wanting to keep it, the test should check
it)

That's a good point. What do you think @mattetti
https://github.com/mattetti?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#322 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAAcYnrncx5hvpuAPtiQgnGuZ-VS3LRks5qvtKXgaJpZM4IRinB
.

kytrinyx · 2016-10-02T00:04:29Z

Unfortunately I don't currently have time to do so myself at the moment.

I hear you! ❤️

If someone has the time to make a PR to this PR that would be awesome! (I added the hacktoberfest label, maybe some friendly gophers can help here)

mattetti · 2016-10-02T15:59:38Z

@Ngo-The-Trung updated the tests and I merged their PR against my branch but it doesn't seem to be reflected here. The new GH review does seem interesting but I'm on mobile and super confused. Sorry :(

kytrinyx · 2016-10-02T17:45:42Z

@mattetti I think it must have been caching, because I see it here.

I'm merging this. Thanks @mattetti @Ngo-The-Trung ❤️

petertseng · 2016-10-02T18:06:29Z

The PR mattetti#1 caused the new commits to show up in your master branch. But this PR was from your ma-variable-length-quantity branch, not your master branch. So the commit didn't get merged to this repo... but that's OK, we can just merge it now.

petertseng · 2016-10-02T18:08:58Z

Applied using https://github.com/mattetti/xgo/commit/4b6df38d1f44ad1e9501acae4a81ce16ef383953.patch + git am, resulting in a5d91a0. Thanks to all involved.

Since it was introduced in #322, the decode function returns the number of bytes decoded. The creator of the exercise mentioned that it was to be consistent with the standard library's API. Go standard library example for returning number of bytes: https://golang.org/pkg/io/#Writer Now, since #674, it is required that the entire input array be decoded, or else it is an error. Now the number of bytes decoded doesn't make much sense to return, since now it is known that it will always be the length of the input. Go standard library example for not returning number of bytes (decoding everything): https://golang.org/pkg/encoding/json/#Unmarshal testVersion -> 4 Closes #683

new exercise to encode/decode variable length quantity

5816820

remove the test/exercise documentation from the exercise since it wil…

914be72

…l be moved to the problem

mattetti mentioned this pull request Apr 28, 2016

new variable length quantity problem exercism/problem-specifications#239

Merged

kytrinyx reviewed Apr 29, 2016
View reviewed changes

provide an implementation example

3464be9

petertseng reviewed Apr 29, 2016
View reviewed changes

kytrinyx added the Hacktoberfest label Oct 2, 2016

Ngo-The-Trung mentioned this pull request Oct 2, 2016

Vle test: add encoding size mattetti/xgo#1

Merged

kytrinyx merged commit ae98780 into exercism:master Oct 2, 2016

This was referenced May 23, 2017

variable-length-quantity: I don't think returning number of bytes decoded is necessary #683

Closed

variable-length-quantity: Decode: don't return/test length #687

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new exercise to encode/decode variable length quantity #322

new exercise to encode/decode variable length quantity #322

mattetti commented Apr 28, 2016

mattetti commented Apr 28, 2016 •

edited

Loading

IanWhitney commented Apr 29, 2016

kytrinyx Apr 29, 2016

kytrinyx Apr 29, 2016

mattetti commented Apr 29, 2016 •

edited

Loading

petertseng commented Apr 29, 2016

mattetti commented Apr 29, 2016

petertseng Apr 29, 2016 •

edited

Loading

kytrinyx commented Oct 1, 2016

mattetti commented Oct 1, 2016

petertseng commented Oct 1, 2016

kytrinyx commented Oct 1, 2016

mattetti commented Oct 2, 2016

kytrinyx commented Oct 2, 2016

mattetti commented Oct 2, 2016

kytrinyx commented Oct 2, 2016

petertseng commented Oct 2, 2016

petertseng commented Oct 2, 2016

new exercise to encode/decode variable length quantity #322

new exercise to encode/decode variable length quantity #322

Conversation

mattetti commented Apr 28, 2016

mattetti commented Apr 28, 2016 • edited Loading

IanWhitney commented Apr 29, 2016

kytrinyx Apr 29, 2016

Choose a reason for hiding this comment

kytrinyx Apr 29, 2016

Choose a reason for hiding this comment

mattetti commented Apr 29, 2016 • edited Loading

petertseng commented Apr 29, 2016

mattetti commented Apr 29, 2016

petertseng Apr 29, 2016 • edited Loading

Choose a reason for hiding this comment

kytrinyx commented Oct 1, 2016

mattetti commented Oct 1, 2016

petertseng commented Oct 1, 2016

kytrinyx commented Oct 1, 2016

mattetti commented Oct 2, 2016

kytrinyx commented Oct 2, 2016

mattetti commented Oct 2, 2016

kytrinyx commented Oct 2, 2016

petertseng commented Oct 2, 2016

petertseng commented Oct 2, 2016

mattetti commented Apr 28, 2016 •

edited

Loading

mattetti commented Apr 29, 2016 •

edited

Loading

petertseng Apr 29, 2016 •

edited

Loading