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

Refactors #192

Merged
merged 15 commits into from
Jan 19, 2025
Merged

Refactors #192

merged 15 commits into from
Jan 19, 2025

Conversation

sftse
Copy link
Contributor

@sftse sftse commented Dec 16, 2024

These are the non-behavior-changing modifications from #191

The original motivation for these changes is to streamline the way the library calls itself internally. We observed some slight mismatches between how the public API exposes the configurable stages and how the library passes information internally. We investigated whether those mismatches were a bug, but thankfully they were not. Still, the changes we suggest make this clearer by using higher-level functions where possible.

src/lib.rs Outdated Show resolved Hide resolved
@sftse sftse force-pushed the refactors branch 2 times, most recently from 01dd84e to edfd1a0 Compare January 5, 2025 14:02
@sftse sftse requested a review from jugglerchris January 5, 2025 14:09
@jugglerchris
Copy link
Owner

I'm sorry it's going to take me longer to review this - since you have force-pushed I can't see a way to easily compare just the changes since I reviewed it before, and as it's been a little while with the holidays I'll have to review it all again.
FWIW I'd prefer just additional commits when revising an MR rather than history rewriting.

@sftse
Copy link
Contributor Author

sftse commented Jan 13, 2025

Yes, that was unfortunate. Will keep in mind for the future.

@jugglerchris jugglerchris merged commit b145707 into jugglerchris:main Jan 19, 2025
5 checks passed
@jugglerchris
Copy link
Owner

Merged, thanks!

@sftse sftse deleted the refactors branch January 19, 2025 12:05
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