-
-
Notifications
You must be signed in to change notification settings - Fork 205
Bumped hamming to 2.3.0 #318
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
Conversation
|
Travis seems to not supprot jdk8 anymore. Updated to jdk11. |
|
@uzilan just so you know, these PRs should count towards your Hacktoberfest progress even if they aren't labelled. The labels are just to help discoverability of suitable issues :) |
|
Ah cool, thanks. These were required last year, I guess they changed it |
| language: java | ||
| jdk: | ||
| - oraclejdk8 | ||
| - openjdk11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be its own pull request. It isn't related to the Hamming exercise, other than to allow Travis to pass, but this is more general than only Hamming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, cool. Created a new PR, #323 for changing the jdk only.
|
We could bring the changes in, I just would not want to lose the history of the commit that updates the JDK. |
|
Thanks @uzilan |
|
Updated test names using backticks, I hope you're fine with this. |
Update my fork
|
Done |
|
Sorry, Slack archive is not available anymore. |
|
Here's the discussion we had: https://gist.github.com/uzilan/2b807a852b802683bca48ad5a387a113 |
|
So basically we've decided to skip the tests that checked for empty strands and instead encourage mentees to use |
|
Got it, thanks! Maybe it's better to implement this tests (but comment-out them?). I'm not sure that we can change test cases and bump used specification versions as our's test cases will not reflect actual specification. |
|
I don’t think Exercism is so rigid that we can’t decide stuff like that on our own but I could add commented tests if you want them |
|
It's just my opinion, but I find partially-implemented specification (without explicit notice) quite misleading for those who will support this repo later. 😅 |
|
Colleagues, I was asked to form and expose my opinion on the case, and here it is. We have two separate things here to discuss, and it's important to distinguish them.
We have an interesting theme (Hamming Distance) and strive to design it to the best task for a student. What useful construction a student will exploit for the first exception demanded? What student will learn on the second exception over the same data, when one cannot use In any solution there are two parts: plot (algorithm and idioms selection for it) and chores. I think, we should keep our student burning by any means we have. Actually there lots of thing in the outer world, that distract them. For our case, IMHO, additional exception messages are chores.
If we do not implement these requirement as tests - why to keep requirements in the first place? I conclude: |
|
@eparovyshnaya Thanks for you thoughts here! Tbh, I'm quite against silent specification violation - because it's like intentionally creating a mess. :) 💭 Should be discussed in another thread, but what if we'll provide some useful links to documentation with each exercise. |
Update my fork
dector
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let it go like this for now. We'll sanitize our tests later.
Bumped hamming to 2.3.0 and changed the tests according to https://raw.githubusercontent.com/exercism/problem-specifications/master/exercises/hamming/canonical-data.json
Left out tests for empty left or right strand after discussion in Slack: https://exercism-team.slack.com/archives/GC3K95MRR/p1570051281069500
Could this PR be labeled with hacktoberfest please?