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

armstrong-numbers: Add hyphens in descriptions #1687

Merged
merged 1 commit into from
Oct 12, 2021

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Oct 13, 2020

This PR fixes a grammatical error. The "three-digit" in "three-digit number" is a compound adjective, and so should have a hyphen. It helps the reader to understand that:

  "three-digit" modifies "number"

rather than:

  "three" modifies "digit number"

There is also one case of this diff:

-      "description": "There are no 2 digit Armstrong numbers",
+      "description": "There are no two-digit Armstrong numbers",

which includes a separate 2 -> "two" replacement because:

  • it should be consistent with the other test cases
  • there's a common style "rule" to use words rather than numerals for small numbers

@ErikSchierboom
Copy link
Member

This is formally correct, as the spec defines the description as also being immutable. I'm thinking if we should consider making it mutable as these new test cases really don't add anything except for the fixed description. The only downside to making this mutable is that most test generators use the description to determine the test name. Changing the description should in theory not break any test generators, although theoretically they could be executing specific logic if the description matches a certain pattern. That said, I don't think test generators should actually be doing that, as they should use either the uuid/property/scenarios property for that (which are immutable).

@iHiD
Copy link
Member

iHiD commented Oct 13, 2020

I'm 👍 on having mutability here for typos, with any contention leading the immutability rules kicking in. I'm also entirely happy to go with immutability as a hard rule for now, and loosen it later if desired/appropriate.

@SleeplessByte
Copy link
Member

Mutable; for the mere fact the the meaning / content / test doesn't change.

@SaschaMann
Copy link
Contributor

SaschaMann commented Oct 13, 2020

That said, I don't think test generators should actually be doing that, as they should use either the uuid/property/scenarios property for that (which are immutable).

I'd also consider this metadata that should be mutable.

There's no guarantee that they make any sense in the language (I remember an old discussion about test names being long and generators turning them into 120 character long method names) or that they even accurately describe what's being tested

I'm 👍 on having mutability here for typos,

I'd rather have a hard rule than any subjectivity. Is "tests that 2^2=4" -> "tests that 2²=4" a typo or not?

@ee7
Copy link
Member Author

ee7 commented Oct 13, 2020

Seems like nobody so far likes the implications of description being immutable.

I think that my opinion is:

  1. We should allow changes to description that do not change the meaning of the test. Otherwise, the canonical-data.json files will end up bloated with all sorts of noise like this PR, and be unpleasant to wade through.
  2. We shouldn't have a strict rule of "a description-only change never requires a new test case". That's because we can probably come up with some pathological case where a description-only change also changes the meaning of the test. But it's probably rare that we do insist that a description-only change should be a new test case.
  3. People can re-run their generators even though a new test case wasn't added.

Anything to do with manipulating UUIDs is also quite error-prone, as we saw with the number of UUIDs that were invalid or non-distinct across v2 tracks (people would implement an exercise, copy part of a config.json from another track and not change the uuid). I know that we should have tooling/CI to check for valid UUIDs, but at some point, we'll likely see things like:

  • the value of a reimplements is a UUID that refers to a different test to the one it reimplements.
  • the value of a comments does not reflect what actually happened for a description-only change.

which are other reasons to reduce the number of reimplements by making description mutable.

@iHiD
Copy link
Member

iHiD commented Oct 13, 2020

I'd rather have a hard rule than any subjectivity. Is "tests that 2^2=4" -> "tests that 2²=4" a typo or not?

I think we should have Exercism-wide rules about these individual situations to guide us.

So for example, if we say "Always spell words out and use dashes to separate from what they describe" (which is correct English) then we can make this change without conflict. Similarly we would make a decision on 2^2 vs , and then apply that consistently.

I guess my feeling is these are Exercism-level-decisions (albeit ones that we can make as a community) that individual tracks shouldn't be opting in or out of.

@ee7
Copy link
Member Author

ee7 commented Oct 13, 2020

If we make description mutable there's a small question about canonical_data_syncer . Say that we:

  1. Change a description for some UUID in a canonical-data.json
  2. Then run canonical_data_syncer on a track with a tests.toml file that contains that UUID, but the old description.

Should it update the tests.toml file to overwrite the comment that contains the description?"

I think the answer is yes, but it currently doesn't.

@iHiD
Copy link
Member

iHiD commented Oct 13, 2020

If we went down the mutable route, then yes it should.

@ErikSchierboom
Copy link
Member

So it seems like most people (including me) are in favor of allowing the description to be mutable. You okay with that @iHiD?

This fixes a grammatical error. The "three-digit" in "three-digit
number" is a compound adjective, and so should have a hyphen. It helps
the reader to understand that:
  "three-digit" modifies "number"
rather than:
  "three" modifies "digit number"
@ee7 ee7 force-pushed the armstrong-numbers-description-add-hyphens branch from 80b4423 to 3f07763 Compare October 14, 2020 10:46
@iHiD
Copy link
Member

iHiD commented Oct 14, 2020

Yes I am. But I think:

  1. For any changes to descriptions, it should be:
    a. Because of a typo (e.g. a spelling mistake) that is undisputable; or
    b. Because of a standard that we are enforcing (my examples in my previous comment).
  2. If someone PRs a change that requires a standard defining (e.g. in this case) we need to document that standard somewhere.

So along with the commit in this PR, I'd also want to see another commit in this PR that says "write numbers as two-digit not 2 digit. I don't know the right place for that though.

@SaschaMann
Copy link
Contributor

I guess my feeling is these are Exercism-level-decisions (albeit ones that we can make as a community) that individual tracks shouldn't be opting in or out of.

I believe that the examples in this thread depend on the context of the exercise and also the idioms around a language. I think a global standard for that would lead to some languages with more modern attitudes towards text encoding for example having to deal with archaic forms of writing for no good reason.

Because of a standard that we are enforcing (my examples in my previous comment).

That seems hard to keep track of, in particular since ps tends to see contributions from students who have not contributed before quite often. How would you enforce such standards?

@iHiD
Copy link
Member

iHiD commented Oct 14, 2020

It's a yard-stick to hold us to in order to avoid commits yo-yo-ing backwards and forward.

Say ee7 prefers "two-digit" and someone else prefers "2-digit". The second person may not notice this PR and then later come across it and then PR to change it back. Without having a firm decision on which is "correct" according to our style-guide, we either end up with commit-paths going round in circles or debates about these things when they're not necessary (and which is why we added immutability in the first place). So if we're making an exception to the immutability rule, it needs to be because it is clear to everyone that a typo/style issue is being fixed, and to achieve that we need those style rules.

If someone commits some new contributor commits something with the wrong style, that's not a problem because the description-mutability rule allows a maintainer to see that it can easily be changed without a new test-case being added.

I believe that the examples in this thread depend on the context of the exercise and also the idioms around a language.

I think that we should be able to agree on a clear way to write things as our default standard. For example "two-digit" is much more correct in English than "2 digit" (I write "2 digit" and correctly get it corrected by others constantly), so it's good to use that. Similarly it seems to me that we should be using either 2^2 or 2² consistently across Exercism.

I think a global standard for that would lead to some languages with more modern attitudes towards text encoding for example having to deal with archaic forms of writing for no good reason.

I think we should be able to document stylistic-preferences which are friendly, clear and non-archaic. I think it's probably pretty clear that Exercism's "voice" isn't going to be "textbook" and archaic. And I'm not suggesting we do this as an up-front exercise, but its something that as exercises evolve we can add to and refine.

@SleeplessByte
Copy link
Member

I still think that if the meaning doesn't change: mutate it. Otherwise new test. But that might not be a clear-cut rule.

@iHiD
Copy link
Member

iHiD commented Oct 14, 2020

I still think that if the meaning doesn't change: mutate it. Otherwise new test. But that might not be a clear-cut rule.

I agree. I just want to really define the details of this to avoid the previous conflicts where things that felt very minor to some people felt very major to other people.

@ErikSchierboom
Copy link
Member

I think we should be able to document stylistic-preferences which are friendly, clear and non-archaic. I think it's probably pretty clear that Exercism's "voice" isn't going to be "textbook" and archaic. And I'm not suggesting we do this as an up-front exercise, but its something that as exercises evolve we can add to and refine.

I think we can definitely come up with some guidelines on how to write things. Like writing out numbers in full when then are less than 20 or something like that.

I still think that if the meaning doesn't change: mutate it. Otherwise new test. But that might not be a clear-cut rule.

I agree. I just want to really define the details of this to avoid the previous conflicts where things that felt very minor to some people felt very major to other people.

And I'm hesitant to introduce rules that are not clear-cut 🤷 I expect description changes to not be prone to a lot of discussion. By far the majority of issues was always on the input/expected/property values, not the description. So I would argue that for now we'll make the description field mutable and see if we actually do end up with issues. If so, we can always add a more restrictive rule.

@ee7 ee7 requested review from ErikSchierboom and iHiD October 16, 2020 15:27
Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

Let's then just agree to merge this. Move the discussion out (we already did that with the guides) and add more rules via PRs later.

@iHiD
Copy link
Member

iHiD commented Oct 20, 2020

@SleeplessByte Agreed.

@ee7 Please could you PR to the style guide about using compound adjectives please. Some example of when to use it would be appreciated. Also, we should stick to zero-ten in words and 11 upwards in numbers when in general text I think. Could you add this too pls.

Base automatically changed from master to main January 27, 2021 15:31
@wolf99 wolf99 merged commit c6884e5 into exercism:main Oct 12, 2021
@ee7 ee7 deleted the armstrong-numbers-description-add-hyphens branch January 18, 2022 12:59
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.

6 participants