Skip to content

Conversation

@mrcfps
Copy link

@mrcfps mrcfps commented Feb 14, 2018

Updates with nothing changed, such as applying new input policy, correcting naming styles, etc:

Updates with changes to tests:

@mrcfps
Copy link
Author

mrcfps commented Feb 15, 2018

@cmccandless While I'm working on updating tests, I've encountered two exercises that differ quite a lot from canonical data:

  • food-chain: Currently there is even no concept of input in the Python implementation, but canonical data has input parameters startVerse and endVerse. Since this exercise resembles so much to house, should I adopt the same test style?
  • binary-search-tree: It seems this exercise had been implemented before canonical data was proposed, so a thorough discussion on updating this exercise is needed. Maybe a new PR?

@cmccandless
Copy link
Contributor

@mrcfps

food-chain: Currently there is even no concept of input in the Python implementation, but canonical data has input parameters startVerse and endVerse. Since this exercise resembles so much to house, should I adopt the same test style?

Yes, I believe adopting a similar style to house, twelve-days, and beer-song would be the best practice.

binary-search-tree: It seems this exercise had been implemented before canonical data was proposed, so a thorough discussion on updating this exercise is needed. Maybe a new PR?

Yes, I recall commenting in #773 that canonical data was being implemented; I'm not sure why I didn't push then to wait until the data was merged to problem-specifications. I think a new PR would be the best practice.

I will review this PR once you've made the changes to house, but ordinarily separate PRs are preferred when different exercises are involved (the exception to this is when the same issue is being addressed in all exercises, such as overhauling exception usage and handling). Separate PRs make it easier for maintainers to review and selectively merge without having to wait on the entire batch of changes to pass review.

@cmccandless cmccandless self-requested a review February 15, 2018 14:57
@mrcfps
Copy link
Author

mrcfps commented Feb 15, 2018

@cmccandless Separating PRs would be a better idea, and thank you for pointing this out. I'll keep this PR as a checklist, and close it when everything is done.

@cmccandless cmccandless removed their request for review February 16, 2018 22:32
@mrcfps
Copy link
Author

mrcfps commented Feb 28, 2018

I think we're done with this wave of tests updates (except react for callback issues).

@mrcfps mrcfps closed this Feb 28, 2018
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.

2 participants