Skip to content

nth-prime: Use object instead of bool for invalid value #969

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 25, 2017
Merged

nth-prime: Use object instead of bool for invalid value #969

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d864927
Use object instead of bool for invalid value in nth-prime
ErikSchierboom Oct 23, 2017
5b67734
Use minor instead of major version bump
ErikSchierboom Oct 23, 2017
bb51d1c
Revert back to original major version bump
ErikSchierboom Oct 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions exercises/nth-prime/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
{
"exercise": "nth-prime",
"version": "1.0.0",
"comments": [
"The tests that expect 'false' should be implemented to raise",
"an error, or indicate a failure. Implement this in a way that",
"makes sense for your language."
],
"version": "2.0.0",
Copy link
Contributor

@Insti Insti Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This change should not cause the version to change to 2.0.0.
This opinion may differ from the documented procedure, see also: #938

Copy link
Member Author

Choose a reason for hiding this comment

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

@Insti Ah, I missed that discussion. I'll make it a minor version update then.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Insti Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not actually update the documented procedure before introducing deviations on a per-exercise basis?

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to pretend that this practice (of changing version numbers in contravention of the written meanings) disturbs me deeply and write this comment accordingly.

If we follow the written rules, even if we disagree with them, we at least know what each version change means.

Now that we are openly flaunting the rules, now we have no idea what each version change means. (It is possible they do have a consistent meaning, but the meaning is not written down)

I cannot* stand by this behaviour because I need my version numbers to have meaning. With these meaningless version numbers, my burden as a maintainer has increased unacceptably.

*: I actually don't care, since I don't have a track for which it matters in what precise way the tests were updated. Right now the only property I need to hold true is that the version changes in some way when the file changes... which I guess I could have achieved using commit shas.

Copy link
Member Author

Choose a reason for hiding this comment

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

@petertseng @stkent @Insti What should I do with this versioning problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

Even on a track that uses not-generated tests, I don't really care about the difference between a major and a minor change, so I say move forward with this as-is and maybe separately open an issue to begin discussion of the current versioning guidelines?

Copy link
Member

@petertseng petertseng Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Because I said in #969 (comment):

I actually don't care

it is thus safe to proceed without asking for my further opinion.

But you asked anyway, so now I have to give it to you.

As I explained, I cannot accept any version change that does not correspond to a written versioning scheme that explains what a change in each component means. Either use 2.0.0 as the current versioning scheme would have it, or explain in writing the new versioning scheme.

Copy link
Member

Choose a reason for hiding this comment

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

does not correspond to a written versioning scheme

Sorry, is the versioning scheme used here that written in #938 (comment)? It wasn't clear to me because that proposal says to remove the third component, which was not removed, so I rejected the possibility that it was that scheme.

"cases": [
{
"description": "first prime",
Expand Down Expand Up @@ -35,7 +30,7 @@
"description": "there is no zeroth prime",
"property": "prime",
"input": 0,
"expected": false
"expected": { "error": "there is no zeroth prime" }
}
]
}