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

Rename Flags EventFlag and MemFdCreateFlag #2476

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

howjmay
Copy link
Contributor

@howjmay howjmay commented Aug 29, 2024

relates to #317

What does this PR do

Rename Flags

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Existing changes look good, thanks!

Though you need to address the CI failures, i.e., rename the usages on other platforms as well.

And can we have something like this to help users migrate:

#[deprecated(since = "0.30.0", note = "Use `EvFlags instead`")]
pub type EventFlag = EvFlags;

@SteveLauC
Copy link
Member

Also, a changelog entry is needed, see this on how to add one.

@howjmay howjmay force-pushed the rename-flag branch 3 times, most recently from eb300f1 to 4be7d73 Compare October 1, 2024 21:22
@howjmay howjmay changed the title Rename Flags: Rename Flags EventFlag and MemFdCreateFlag Oct 1, 2024
@@ -11,7 +11,7 @@ fn test_struct_kevent() {
let actual = KEvent::new(
0xdead_beef,
EventFilter::EVFILT_READ,
Copy link
Member

Choose a reason for hiding this comment

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

@asomers, any idea what we should do to things like this enum EventFilter?

use nix::sys::event::{EvFlags, EventFilter};

Honestly, it feels weird to me when putting them together 🥲, one aims to be short, typical C-style, one aims to be self-explanatory.

Copy link
Member

Choose a reason for hiding this comment

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

If does seem odd now. I think they'll need hard to remember like this. We should probably make the two names more consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah

Copy link
Member

@SteveLauC SteveLauC 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, thank you:)

@howjmay
Copy link
Contributor Author

howjmay commented Nov 16, 2024

thank you too

@SteveLauC
Copy link
Member

Let's merge this first.

@SteveLauC SteveLauC added this pull request to the merge queue Nov 17, 2024
Merged via the queue into nix-rust:master with commit 7452b68 Nov 17, 2024
39 checks passed
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.

3 participants