Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trinary: Add canonical-data.json #510

Merged
merged 1 commit into from
Jan 28, 2017

Conversation

bunnymatic
Copy link
Contributor

Added data for trinary exercise.

Included 3 tests around invalid trinary strings. This is basically a union of all invalid tests that I saw across different tracks and it matches with the exercise description.

This addresses this issue on the todos repo: https://github.com/exercism/todo/issues/158

{
"description": "invalid trinary digits returns 0",
"input": "1234",
"expected": 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Looks like binary and trinary started the same, but trinary didn't get the af4e4a0 treatment. That's too bad. I do dislike to encourage behaviour that can't distinguish between valid and invalid inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make these { expected: { error: "invalid input" }}

I suppose as I've been doing this work of generating the canonical, i wonder how much I'm supposed to make it match the tests that exist or if it's supposed to become the "rule" and i should just expect that the tests that are out there that don't match will someday get updated. It becomes about how much of these should be "what we think the canonical test suite should look like" and how much of it is "what the majority of tests say the canonical test suite is right now"

@ErikSchierboom
Copy link
Member

Ehm, isn't trinary a deprecated exercise? See #279.

@bunnymatic
Copy link
Contributor Author

bunnymatic commented Jan 25, 2017

Ha!

I just pulled it for ruby yesterday (with a fetch). After doing the exercise and realizing that I'd also done the Java version, I thought i might as well do the canonical.

So once those are "deprecated", how do we remove them from the existing tracks?

What about adding a DEPRECATED.md file or something in the x-common repo as an indicator?

Either way, just trying to help out.

I'll close this guy.

@bunnymatic bunnymatic closed this Jan 25, 2017
@bunnymatic
Copy link
Contributor Author

I suppose we should close exercism/todo#158 if this is going to be unused

@ErikSchierboom
Copy link
Member

Oh, you didn't need to close it yet. I'll keep it open until @petertseng or @kytrinyx has something to say about.

@bunnymatic
Copy link
Contributor Author

UNCLOSE!

@bunnymatic bunnymatic reopened this Jan 25, 2017
@petertseng
Copy link
Member

petertseng commented Jan 25, 2017

I take the same position as #296 (comment) - I don't oppose its addition. I recommend not taking my word as having any more authority than any other track maintainer though! =D

I say that because other time-commitments prevent me from pushing a cross-track initiative to have all tracks deprecate their existing base-conversion exercises. If there is such an individual pushing the initiative, that individual should recommend that this file not be added!

So once those are "deprecated", how do we remove them from the existing tracks?

All tracks that have added the all-your-base exercise should add the other base-conversion exercises to the deprecated section of the track config file. This stops exercism fetch from fetching the exercise, unless specifically requested to do so by specifying it. A month and a half ago I made that list of tracks that had alerady done so: #279 (comment)

Global deprecation is performed by adding a .deprecated file in the directory in x-common. Someone will need to remind me what that does. https://github.com/exercism/x-api/issues/139 says it means it doesn't show up in todo lists, please confirm whether that's true, and how it interacts with the per-track deprecation (if it does at all...!)

It will be worthwhile, however, to state the impending deprecation in the todo issues for the remaining two, hexadecimal and octal, so that it is known that other problems can take priority. I have done so.

how much of these should be "what we think the canonical test suite should look like" and how much of it is "what the majority of tests say the canonical test suite is right now"

I think of it as the former. In the investigation, we may discover that the majority test suite is a good one, in which case the two will be the same, but we would generally like to take the opportunity to make sure that the test suite is one we are happy with when we add the tests to this repo.

I know sometimes I want to argue "hey listen, I just submitted this PR to show what the current tests look like. I'm really not interested in making any changes y'all want to suggest. I just want this PR to be over with.", but I do think it is part of the work involved in doing these.

This may be important enough to document.

@bunnymatic
Copy link
Contributor Author

Thanks @petertseng . I guess I'll give it a little time. We can see If @kytrinyx weighs in. Though based on the comment you pointed to, it sounds like we're leaning towards merging it in, and letting the deprecation (which happens later) take care of dumping it, right? Is that what I heard?

Interestingly, after reading comments from @Insti here, and above, I want to rewrite the whole thing so that the test descriptions make more sense and maybe there are fewer tests because what are we testing really with all those cases..

But if the exercise is deprecated, then I think... who cares ;)

I'm happy to close this one and leave it unfinished or whatever.

Just let me know.

@Insti
Copy link
Contributor

Insti commented Jan 26, 2017

Unless you really want to, I'd leave this one as it is and not worry too much about improving it. There are many other things that would be a better use of your time.

@bunnymatic
Copy link
Contributor Author

I'm happy to leave this alone. And if it gets merged, fine. If not, fine.

@kytrinyx
Copy link
Member

I think based on the above discussion: (1) let's not worry about improving it, but also (2) let's not throw it away. Also, eventually, we should probably take the time to figure out exactly what work needs to happen to properly deprecate everything. Are we missing issues notifying all the various tracks about it?

@kytrinyx kytrinyx merged commit 851013b into exercism:master Jan 28, 2017
@petertseng petertseng mentioned this pull request May 6, 2017
emcoding pushed a commit that referenced this pull request Nov 19, 2018
* Generate tests for "all your base"

* Move private row attribute to avoid ruby warning

This change is to remove a warning present in ruby versions
less than 2.3.0. A thread discussing this warning can be found
at https://bugs.ruby-lang.org/issues/10967. It is removed in
ruby/ruby@32a5a09.

* A bit more idiomatic Ruby

Also, the private call was ineffective.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants