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.

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

Conversation

ErikSchierboom
Copy link
Member

This is more in line with how other exercises indicate error cases.

"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

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

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.

@Insti Insti changed the title Use object instead of bool for invalid value in nth-prime nth-prime: Use object instead of bool for invalid value Oct 23, 2017
@Insti
Copy link
Contributor

Insti commented Oct 23, 2017

The actual change is good 👍

Insti
Insti previously approved these changes Oct 25, 2017
@Insti Insti dismissed their stale review October 25, 2017 10:56

I'm not sure it's reasonable that this PR is being used as a version numbering scheme battleground. I'm fine with the change being meged but I don't 'Approve' of the major version chnage.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

you don't need my approval, but here it is anyway, because I back up my previous statement that I support 2.0.0, being what the current version scheme says. This statement is not to be taken as support for or opposition against the current version scheme.

@Insti Insti merged commit e82cbcd into master Oct 25, 2017
@Insti Insti deleted the nth-prime-error-value branch October 25, 2017 15:15
@Insti
Copy link
Contributor

Insti commented Oct 25, 2017

Discussion about versioning and updating the documented guidelines can continue in #938.

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.

4 participants