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

49 - allow read_index_with_flags for memory mapping some index types #50

Merged
merged 6 commits into from Mar 28, 2022

Conversation

mooreniemi
Copy link
Contributor

@mooreniemi mooreniemi commented Feb 25, 2022

I think using pub mod consts here achieves least surprise for users and minimizes boilerplate to make enum like a bitmask. (There's this crate, and this crate, but it seems a lot just to make enum have something like impl BitOr / allow for "and"ing options.)

I could attempt to test this by creating a temporary index file and open it with these flags, too.

Not sure if you'd really want to version bump on this.

src/index/io_flags.rs Outdated Show resolved Hide resolved
Copy link
Owner

@Enet4 Enet4 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 for the pull request. Please see the concerns inline. I suspect some parts were not written to completion.

src/index/io.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/index/io_flags.rs Outdated Show resolved Hide resolved
@mooreniemi
Copy link
Contributor Author

Hi just bumping on this. I responded to your comments and can't proceed without more feedback.

src/index/io.rs Outdated Show resolved Hide resolved
src/index/io.rs Outdated Show resolved Hide resolved
src/index/io_flags.rs Outdated Show resolved Hide resolved
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Right, please look into the duplicate definition problem and see if you can incorporate the IoFlags newtype. With that done, I think this would be ready to enter.

@mooreniemi
Copy link
Contributor Author

Sorry for delays, and thanks very much for your feedback, was great to learn proper way to do the BitOr here.

@mooreniemi
Copy link
Contributor Author

Do you think this can go up as a new crate version in the next week? I'd like to take a dependency on this functionality soon.

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Yes, a new crate version is due. I'll look into this. Thank you for your contribution!

@Enet4 Enet4 merged commit d4086ec into Enet4:master Mar 28, 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.

None yet

2 participants