Skip to content

Changes to Xorcism discussed in #1040 #1041

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

Merged
merged 8 commits into from
Jan 4, 2021
Merged

Changes to Xorcism discussed in #1040 #1041

merged 8 commits into from
Jan 4, 2021

Conversation

snoyberg
Copy link
Contributor

This should implement all of the changes discussed in #1040, barring one. Outside of a comment around impl Trait and removing the link to ExactSizeIterator, I did not modify the instructions in README.md. I believe with the change in tests to focus on munge_in_place first, the user will have sufficient ramp-up experience implementing the simpler exercise before jumping into the more complex one.

@coriolinus
Copy link
Member

It's going to be at least another day until I can review this properly--we're well into New Year's Eve now, in my time zone at least--but I can say that the failing CI is because you edited README.md directly. Instead, do this:

  • make an appropriate edit / insert into .meta/hints.md
  • get configlet, probably with rust/bin/fetch-configlet
  • run configlet generate . -o xorcism to regenerate the actual README

That should sort out the CI failure.

@snoyberg
Copy link
Contributor Author

snoyberg commented Jan 1, 2021

Thank you for the pointer! I'll try to get CI fixed. And no pressure at all from me, have a great New Year's.

@coriolinus coriolinus linked an issue Jan 3, 2021 that may be closed by this pull request
Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Looks good! One nit remains that I'd like to see fixed before merging, but overall, I'm happy with this PR.

I have reviewed all changes made up to this point, and confirmed that it addresses all points raised in #1040. I have not yet run this locally to verify that bonus tests still pass (IIRC, our CI doesn't yet know how to handle those), but I see no reason why they wouldn't.

@snoyberg
Copy link
Contributor Author

snoyberg commented Jan 4, 2021

OK, the build is fixed. Let me know if you'd like me to rebase at this point.

Copy link
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I have now verified locally that all tests, including feature tests, pass.

@coriolinus coriolinus merged commit 13cc53f into exercism:master Jan 4, 2021
@snoyberg
Copy link
Contributor Author

snoyberg commented Jan 4, 2021

Awesome, thank you! I really appreciate all of the work you and the team put into the exercises here. It was invaluable when I was learning Rust, and has been a huge asset in my teaching.

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.

Confusion with Xorcism exercise
3 participants