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

Sync All your base #694

Merged
merged 6 commits into from
Apr 28, 2024
Merged

Sync All your base #694

merged 6 commits into from
Apr 28, 2024

Conversation

mk-mxp
Copy link
Contributor

@mk-mxp mk-mxp commented Apr 20, 2024

This will be the #48in24 exercise on 2024-04-30. It is marked as ready to sync in the forum. Important files changed, re-running tests on submitted student code is required.

  • Some test inputs differed from the problem spec, but did not change expected behaviour.
  • Students interface design expected null as return value for errors. This is changed to exceptions to align with modern PHP track interface design.
  • Also some test (zeros and leading zeros) behaved different from problem spec expectations. As these values are not mentioned in instructions and so have no defined behaviour, we chose to conform to problem specs (return [0]).

Closes #678

@mk-mxp mk-mxp self-assigned this Apr 20, 2024
@mk-mxp mk-mxp requested a review from homersimpsons April 20, 2024 16:06
Copy link

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

@mk-mxp mk-mxp added x:action/sync Sync content with its latest version x:knowledge/elementary Little Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:type/content Work on content (e.g. exercises, concepts) x:size/small Small amount of work x:rep/small Small amount of reputation labels Apr 20, 2024
@homersimpsons
Copy link
Contributor

@homersimpsons What do you think? Should I invalidate existing solutions to align with problem spec? This would open the way for Exceptions (InvalidArgumentException), too.

I would vote in favour of invalidating existing solutions to align with problem specifications.

Advantages (of aligning):

  • 48in24 will certainly bring some new solutions that would help to fill community solutions with correct ones
  • Updating in the future may be worse
  • It will certainly be easier to sync later

Inconveniences (of aligning):

  • students that want to check existing community solutions may not find what they were looking for
  • some students will see their solutions broken

But I would prefer another point of view from @ErikSchierboom, what do you think about aligning the tests with the spec and invalidating existing solutions ? Maybe there is a correct way to approach this like a message or something to nudge the students to update their solutions and point them in the correct direction (I know they will at least see the diff though).

@homersimpsons
Copy link
Contributor

@ErikSchierboom looks like there was a misunderstanding. The current changes are not aligning the test behaviour with canonical data, it is not throwing exception and won't invalidate existing solutions.

Do you approve the changes as they are now and reject the alignment of the tests ?

@ErikSchierboom
Copy link
Member

Ah, I misunderstood. I'll have to take another look Tuesday then

@mk-mxp
Copy link
Contributor Author

mk-mxp commented Apr 21, 2024

To put in my opinion: I'm very much for updating and modernizing this exercise (and all others), sync-ing to problem specs as much as possible and throwing exceptions as is done in modern PHP everywhere. I read a lot about not invalidating existing solutions, as they are the knowledge base to lookup help / improvements. And yet, how helpful are solutions that follow outdated practices? So to me, updating is the way to go. But I am quite as unsure as @homersimpsons if that's the Exercism way to handle that.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

If I'm seeing this correctly, this PR doesn't break any existing solutions, right?

@mk-mxp
Copy link
Contributor Author

mk-mxp commented Apr 23, 2024

If I'm seeing this correctly, this PR doesn't break any existing solutions, right?

@ErikSchierboom Yes, I didn't break anything, yet. But I would like to do so. Changing behaviour for zero and leading zeros would invalidate all solutions, as we treat that as error now (while problem spec wants [0]). Using exceptions instead of null would invalidate all solutions, too. Shall I break nothing or everything?

@ErikSchierboom
Copy link
Member

I've checked how many solutions there are for this exercise, and it is not a ton so I'm fine with changing the exercise to be more idiomatic. I'm not a fan of the maintainer/contributor-specific changes this PR introduces which might confuse the student.

@mk-mxp mk-mxp added x:size/medium Medium amount of work x:rep/medium Medium amount of reputation and removed x:size/small Small amount of work x:rep/small Small amount of reputation labels Apr 26, 2024
Copy link
Contributor

@homersimpsons homersimpsons left a comment

Choose a reason for hiding this comment

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

Looks better like this, thank you!

@mk-mxp mk-mxp merged commit d2a5961 into exercism:main Apr 28, 2024
10 checks passed
@mk-mxp mk-mxp deleted the sync-all-your-base branch April 28, 2024 08:52
@homersimpsons homersimpsons mentioned this pull request May 3, 2024
4 tasks
tomasnorre pushed a commit to tomasnorre/exercism-php that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/sync Sync content with its latest version x:knowledge/elementary Little Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:rep/medium Medium amount of reputation x:size/medium Medium amount of work x:type/content Work on content (e.g. exercises, concepts)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#48in24: all-your-base 2024-04-30
3 participants