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

feat(cursor): Derive multiple traits for "SetCursorStyle" #909

Merged
merged 10 commits into from
Aug 9, 2024

Conversation

linrongbin16
Copy link
Contributor

@linrongbin16 linrongbin16 commented Aug 7, 2024

This PR derives multiple useful traits for SetCursorStyle enum:

I checked other enums in crossterm, and seems they all have these traits. So I think it's ok to also add to SetCursorStyle.

It also adds 1 unit test to make sure they're working.

@linrongbin16 linrongbin16 requested a review from TimonPost as a code owner August 7, 2024 06:59
@linrongbin16 linrongbin16 changed the title feat(cursor): Derive "Debug", "PartialEq", "Eq" for "SetCursorStyle" feat(cursor): Derive multiple traits for "SetCursorStyle" Aug 7, 2024
@joshka
Copy link
Collaborator

joshka commented Aug 8, 2024

I'm curious what use case there would be for ordering these? I can understand hashing them to store in some lookup table perhaps, but I can't think up a way that ordering them would be useful.

@linrongbin16
Copy link
Contributor Author

linrongbin16 commented Aug 8, 2024

I'm curious what use case there would be for ordering these? I can understand hashing them to store in some lookup table perhaps, but I can't think up a way that ordering them would be useful.

I simply read other enums in this repo, and found most of them derives the PartialOrd and Ord. So I also add them.

But you're right, after a second thought, the PartialOrd and Ord don't have any meaning here.


Update: I removed the PartialOrd and Ord from SetCursorStyle derives. @joshka please take a look.

Copy link
Collaborator

@joshka joshka left a comment

Choose a reason for hiding this comment

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

LGTM other than the comment about removing the tests.

src/cursor.rs Outdated Show resolved Hide resolved
Co-authored-by: Josh McKinney <joshka@users.noreply.github.com>
@linrongbin16
Copy link
Contributor Author

LGTM other than the comment about removing the tests.

Update: accepted the suggestion.

@joshka joshka merged commit b056370 into crossterm-rs:master Aug 9, 2024
6 checks passed
@linrongbin16 linrongbin16 deleted the feat-setcursorstyle-debug branch August 9, 2024 07:38
@linrongbin16

This comment was marked as resolved.

@crossterm-rs crossterm-rs deleted a comment from Schwa10 Nov 1, 2024
@joshka
Copy link
Collaborator

joshka commented Nov 1, 2024

@linrongbin16 thanks - reported user, deleted comment.

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