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.

Sign up for GitHub

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

Jump to bottom

Rename rotation functions #78

Merged
merged 2 commits into from
May 2, 2023
Merged

Rename rotation functions #78

merged 2 commits into from
May 2, 2023

Conversation

SpecificProtagonist
Copy link
Contributor

Fixes #76

Open question: Should the functions be

  • spelled out: clockwise/counter_clockwise
  • abbreviated: cw/ccw
  • abbreviated only in compounds: e.g. clockwise() and rotate_ccw_around()

@SpecificProtagonist SpecificProtagonist mentioned this pull request May 1, 2023
Closed
@alice-i-cecile
Copy link
Collaborator

abbreviated only in compounds: e.g. clockwise() and rotate_ccw_around()

This gets my vote :) Don't feel strongly though.

@alice-i-cecile alice-i-cecile added usability Make the APIs easier to use breaking-change A breaking change that requires a new major version labels May 1, 2023
@ManevilleF
Copy link
Owner

ManevilleF commented May 2, 2023 •
edited
Loading

I like the cw and ccw names but I think they lack clarity and could make the code a bit hard to debug as both methods have such similar names.

Also the << and >> operators exist to allow more visual and short handed rotation.

I agree with @alice-i-cecile, you can add cw and ccw doc alias for clockwise and counter_clockwise methods, and you can abbreviate in compund methods (option 3)

Also I think Hex rotation methods should be edited as well

@SpecificProtagonist
Copy link
Contributor Author

I agree with @alice-i-cecile, you can add cw and ccw doc alias for clockwise and counter_clockwise methods, and you can abbreviate in compund methods (option 3)

Done :-)

Also I think Hex rotation methods should be edited as well

They were, did I miss any?

ManevilleF
ManevilleF requested changes May 2, 2023
View reviewed changes
Copy link
Owner

@ManevilleF ManevilleF left a comment

Choose a reason for hiding this comment

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

One small error and some suggestions for the deprecation warnings. Then we are good to go !

src/direction/diagonal_direction.rs Outdated Show resolved Hide resolved
src/direction/diagonal_direction.rs Outdated Show resolved Hide resolved
src/direction/diagonal_direction.rs Outdated Show resolved Hide resolved
src/direction/diagonal_direction.rs Outdated Show resolved Hide resolved
src/direction/diagonal_direction.rs Outdated Show resolved Hide resolved
src/hex/mod.rs Outdated Show resolved Hide resolved
src/hex/mod.rs Outdated Show resolved Hide resolved
src/hex/mod.rs Outdated Show resolved Hide resolved
src/hex/mod.rs Outdated Show resolved Hide resolved
src/hex/mod.rs Outdated Show resolved Hide resolved
@SpecificProtagonist
Copy link
Contributor Author

Commited, thanks!

@ManevilleF
Copy link
Owner

@alice-i-cecile are those changes okay for you?

@alice-i-cecile alice-i-cecile merged commit 48331cd into ManevilleF:main May 2, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@ManevilleF ManevilleF ManevilleF approved these changes

Assignees
No one assigned
Labels
breaking-change A breaking change that requires a new major version usability Make the APIs easier to use
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unclear rotation names
3 participants
@SpecificProtagonist @alice-i-cecile @ManevilleF