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

Support for read-only files #13

Merged
merged 2 commits into from
Nov 25, 2019
Merged

Support for read-only files #13

merged 2 commits into from
Nov 25, 2019

Conversation

mizvyt
Copy link
Contributor

@mizvyt mizvyt commented Nov 21, 2019

Code reused and improved upon from:

  • Added read_only property to BloomFilter to indicate whether the created bloom filter is read-only.
  • Updated documentation

@prashnts

Copy link
Owner

@prashnts prashnts left a comment

Choose a reason for hiding this comment

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

LGTM except a tiny nitpick.

self._closed = 0
self._in_memory = 0
self._oflags = os.O_RDWR
Copy link
Owner

Choose a reason for hiding this comment

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

Can we either rename it to self._O_FLAGS or add a note about what _oflags is? It took me a few minutes to figure it out!

Copy link
Contributor Author

@mizvyt mizvyt Nov 22, 2019

Choose a reason for hiding this comment

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

I am slightly wary of naming non-consts with uppercase.
I'll add an explanation above the definition?

Basically this needs to be initially set to os.O_RDWR before the if filename is NoConstruct: block is called, since otherwise, when copying a read only template, it would preserve the permissions ("r") and would not allow to create / write to the copy.

Then, after mode is checked, I do self._oflags = _construct_mode(mode).

Let me know if this approach seems okay to you.

Copy link
Owner

Choose a reason for hiding this comment

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

Well it was a nitpick since I didn't find anything else wrong with the changes here!

Don't bother with changing it just for the sake of it. Only need to figure out the merge conflict and let's get this and #14 out in v0.5.0.

@mizvyt
Copy link
Contributor Author

mizvyt commented Nov 24, 2019

I've rebased with upstream/master. Not sure which way is preferred by yourself, but I usually do rebase to update my branch in fork.

@mizvyt mizvyt added this to the next milestone Nov 25, 2019
@prashnts prashnts merged commit a0a75fc into prashnts:master Nov 25, 2019
@mizvyt mizvyt deleted the read_only branch November 26, 2019 01:39
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