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

Fix mistake in Angle::wrap #621

Merged
merged 1 commit into from
May 23, 2022
Merged

Fix mistake in Angle::wrap #621

merged 1 commit into from
May 23, 2022

Conversation

gabsi26
Copy link
Contributor

@gabsi26 gabsi26 commented May 23, 2022

Sorry for the inconvenience, but I made a mistake in the wrapping function, where it previously simply flipped the sign of a negative angle. Now it should properly "overflow" from 2*pi

@gabsi26 gabsi26 requested a review from hannobraun as a code owner May 23, 2022 14:44
@gabsi26 gabsi26 closed this May 23, 2022
@gabsi26 gabsi26 reopened this May 23, 2022
hannobraun
hannobraun previously approved these changes May 23, 2022
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you, @gabsi26!

Sorry for the inconvenience

Don't worry about it! I didn't notice it either. I looked right at that code during review, and my brain just went like "looks legit" 😄

Mistakes happen, and I'm glad you noticed and fixed this one. No need to apologize for that!

@gabsi26
Copy link
Contributor Author

gabsi26 commented May 23, 2022

Thanks for the short feedback btw.
And again sorry for messing up some stuff I honestly have no idea why this one simple line doesn't want to be formatted correctly but I'll just leave it now.

@hannobraun
Copy link
Owner

@gabsi26 You can run cargo fmt locally, which should just take care of the formatting automatically. This requires rustfmt to be installed, which should have happened automatically, due to our rust-toolchain.toml file.

@hannobraun
Copy link
Owner

(looks like we've been posting in parallel 😄)

And again sorry for messing up some stuff I honestly have no idea why this one simple line doesn't want to be formatted correctly

The formatting system works best, if you have the code formatted automatically. You can do this by running cargo fmt before a commit, or by configuring your editor accordingly (I'm using VS Code, for example, and have it configured to format every time I save a file).

The formatting check in the CI build is just meant as the last line of defense. It can get annoying, if you don't use it in combination with a local solution.

but I'll just leave it now.

No worries! I'll take care of it.

@hannobraun
Copy link
Owner

Ran cargo fmt and squashed all the commits. Should be good to go now.

Thanks again, @gabsi26!

@hannobraun hannobraun enabled auto-merge May 23, 2022 15:02
@hannobraun hannobraun merged commit 31ccc29 into hannobraun:main May 23, 2022
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