Leap: Improve the specification so that code generation is more readable - …

[macta]

Improve the description so we can generate test methods like: testYearNotDivisibleBy4InCommonYear vs. testYearNotDivisibleBy4CommonYear (replace the use of : by the word in)

[macta]

I would also like to consider renaming the property "leapYear" to "isLeapYear" as this is what most languages would view as a convention for a boolean result?

[rpottsoh]

I think these changes would only be considered a patch change.

Suggested change

	"version": "1.6.0",
	"version": "1.5.1",

[macta]

I considered that - expect your spec for version changes (the one mentioned in the linter, which failed for me) indicates that the final .x is for patch changes (and this isn't a patch change)? Happy to go with this, but we should also adjust those docs too.

[rpottsoh]

I am not sure I understand what you are saying. I can see that Travis CI failed after the 2nd commit to this PR because "version" was not changed. Branch leap-year-code-gen-improvement maybe should have been rebased onto master after #1465 was merged, it looks like master was merged into leap-year-code-gen-improvement instead. This is when Travis CI failed. bin/check_versions only confirms if the version has been changed. I assume this is what you are speaking about; the third commit reflects the version change. So far this PR has only changed the descriptions which is defined as a patch change. If leapYear becomes isLeapYear that would constitute a major version change.

[macta]

Sorry it was a bit late and I was too brief - when I talked about "patch" I wasn't referring to my branch mis-step - I was referring to the very nice lint checking error message I got: You can see the versioning policy at https://github.com/exercism/problem-specifications#test-data-versioning. In there is talks about the patch version (that last digit). But reviewing it again, I see I missed the last part of the sentence where its not just for bug fixes but
"whenever the meaning of the tests does not change.". I didn't read it properly, and 1.5.1 is better.

Thanks for your patience on all this. In retrospect however, I think these newances aren't worth the effort for either of us - we have much bigger fish to fry. I 'll finish this one but will hold back on future ones.

[rpottsoh]

Improve the description so we can generate test methods like: testYearNotDivisibleBy4InCommonYear vs. testYearNotDivisibleBy4CommonYear (replace the use of : by the word in)

I am impartial to the suggested changes to the case descriptions but I am left wondering why the test generator can't be programmed to convert "year not divisible by 4: common year" to testYearNotDivisibleBy4InCommonYear? You will find that not always will the description be perfect for conversion to legal method names and that some measures on the generator's part should take place to cope with the idiosyncrasies of case descriptions.

[macta]

@rpottsoh with regards to handling ":" - how far do you have to go - and how does it ensure you get a readable test out of the description. The point is - the specs are supposed to be written with code generation in mind (otherwise why bother with a fixed format). As I go through the 105 tests i've generated, I've spotted a few more in this kind of category where the authors weren't thinking about how this might translate programatically and readable (although a large part of them have). It seems a shame not to fix them up as I go by. But if its a useless exercise - no problem, I'll correct them locally and leave it to the next track author to deal with too.

Similarly - my comment about a property called leapYear - yeah its ok, but I would hazard that most language conventions would expect that to be isLeapYear - and yes you can check the return type and if its boolean add an is - if it doesn't already have one. But again, we could just make it readable and easy (for now, I've left it).

I'm thinking that maybe there isn't the appetite for this - so perhaps we just ditch this idea and I can plough through my generated tests (as it is, most other tracks have just manually paraphrased the specs from what I can see)

[macta]

Note: If you would rather not create an update on this exercise - i'm cool with deleting this PR (I've usefully learned from the experience and won't be upset)

[rpottsoh]

As I originally stated, I am impartial to these changes. However, changes to this track potentially affect all the tracks so it is not unusual for a discussion to take place. Hopefully other track maintainers that are also utilizing test generators will voice an opinion on this matter. I am currently not using a generator so these proposed changes to the descriptions is not of huge consequence to me.

We essentially just need input from more people regarding the need/desire for these changes.

[macta]

Ok - lets see if anyone else is generating and how they handle good test names - or whether they feel its worth improving at the spec level to make it easier. Depending on that feedback, we can decide whether its worth improving any of the others (as mentioned - I think there are about 10% that have similar awkward formulation - particularly those with cases inside cases)

[ErikSchierboom]

Here's the perspective of someone that uses test generators :) I like both the suggested changes, as well as the re-naming of the property (which should be a separate PR).

[macta]

Here's the perspective of someone that uses test generators :)

@ErikSchierboom do you periodically re-run your generator to pick up new tests/changes? I haven't got that far yet on the Pharo track - but could conceivably do it (v2 ;)

[ErikSchierboom]

@macta I do. I just have a to execute a single command (dotnet run) from the "generators" directory, and all the test suites are re-generated.

[ErikSchierboom]

@rpottsoh Are you still impartial to this change? If so, we can consider merging this.

[rpottsoh]

Yes. Thanks for asking.

[ErikSchierboom]

Squashed and merged! Thanks @macta!