leap: More test cases for years in the past (less than four digits) #871
Conversation
…fact that years might not necessarily only be four digits adding tests for year in the past might assist the aspiring Rust programmer
coriolinus
changed the title
|
This is a good idea, but my inclination is to not review this while exercism/problem-specifications#1581 is pending. Once that's merged, it would be good to add/adjust tests as required to bring us into compliance with the spec, instead of inventing our own tests. |
|
I was thinking about interesting corner cases and the year The following test case, could be considered for addition. assert_eq!(leap::is_leap_year(0), false);At the same time, it does make the solution more complex, but also demonstrates good test and coding practice. The value |
Is the year 0 legitimate? I thought it did not exist in the Gregorian calendar. I know it does in other calendars. |
There was a problem hiding this comment.
The exercise description is not for Julian calendar, but for the Gregorian calendar. Staying away from (and mentioning that these rules did not exist) years prior to 1752 is probably a better idea. Different areas adopted the Gregorian calendar at different times as well, but this is not the focus of the exercise. We probably want to keep it the naive exercise that it is.
There was a problem hiding this comment.
Now that exercism/problem-specifications#1581 is merged and I've had time to look at these both in some detail, I see that they're not actually covering the same territory. While it would be nice if this PR were to update this exercise to comply with the specifications changes made in that PR, it is not essential.
I agree that the domain of the calculation includes all positive integers. While nobody in the year 100 would have performed this leap year calculation, that doesn't preclude us from doing it.
I'm requesting changes because I think these should definitely not be bundled into the "any old year" test. I would prefer individual cases for each of these additions, but would be willing to accept a new combined "early years" test.
And thank you @jonasbn for doing this work, and for your patience!
There was a problem hiding this comment.
This looks good to me. Thanks, @jonasbn!
@kotp, you do have veto power if you think we really shouldn't add tests for early years. It sounds like your objections are related to the philosophy of dates rather than the actual implementation.
However, if there are no objections, I intend to merge this not earlier than next Wednesday, the 25th.
Make no mistake, I appreciate the work as well @jonasbn even if I have reservations about introducing the early years, pre-adoption. And especially since it will effect, potentially, all the tracks that use this exercise.
Yes, the objections are strictly on the subject of Dates, it is not the goal of the exercise to adequately address years prior to 1572, nor address regional adoption times. The ability to talk about this, especially if a student mentions that some year is a leap year, we can surely advise them about the core or standard libraries available (whatever the term is for the language) that there is a lot of work done to help us ensure that we get things mostly right, by using those tools. This exercise is more about how to manage the logic statement, than actually compute "leap year" to any degree of reliability. If we add information that relates that the tests do not drive a canonical definition, and that we can't reasonably answer "bar bets" with this tool, or we don't mention it and also don't test early years. The exercise is eye opening enough when there is a curious student that tends to ask questions about centuries, such as 1500 and beyond... I generally refer them to check answers against the implemented tooling in the core and standard libraries, to see how the naive leap year solution measures up. And as an educational outlet, we should be careful to not "outright lie" as someone may find themselves arguing one day "But I learned that year XXX was a leap year, because the tests from Exercism had it like that." An interesting read about Julian calendar dates, and Gregorian calendar dates is here. It is usually where I start people reading when they express an interest. At least for the last decade or so. |
|
We might wonder if it would be interesting to "toggle the rules to Julian" if the lower bound does hit 1751 and lower... that may be an interesting aspect, and would provide a pointer to information regarding date. That is about where I would want to leave the tests. |
|
@kotp I am interpreting your statements as "I have reservations, but not enough to veto this PR." I agree that as educators we should avoid falsehoods. However, I believe simplifications are permissible. Example; we have several exercises talking about crypto, and no student should ever use any implementation of those to do actual cryptography with. To me, sticking with the simple definition of a leap year is an allowable simplification. Testing a wide range of years is expanding coverage of our simplification, not arguing that the simplification actually works in all times and places. |
|
OK, but that isn't what I said. And given your example, we are missing the "warnings" that are similar to the warnings we should be giving people on the "crypto" exercises as well. Don't use this for years prior to Gregorian Calendar years, and don't use Crypto that you devise for doing crypto. The simple definition of leap years was the Julian calendar, not the "slightly less simple" definition of what this is. But no, I wouldn't veto, the tests give us talking points for the students that want to explore it a little deeper. Otherwise we should take away the mentioning of the Gregorian calendar, since our rules now don't have that lower bound. No, I wouldn't veto the change, but now, given your example, we should give fair warning, just as we would for a cryptographical exercise. |
Having observed basic solutions on other Rust exercises ignoring the fact that years might not necessarily only be four digit, bys adding tests for years in the past might assist the aspiring Rust programmer.