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

Fix inconsistent file mode formatting and comparison (rb+ changed to r+b) #1426

Merged
merged 6 commits into from
Nov 14, 2023

Conversation

lobis
Copy link
Contributor

@lobis lobis commented Nov 13, 2023

Triggered by #1425

The current processing of the file mode string causes the b character to always be on the end of the mode e.g. rb+ becomes r+b.

However in some instances (such as in the memory file system), the valid file modes listed are not compatible with this format, for instance rb+ is listed. This comparison will never take place given how the mode string is processed.

This PR replaces all instances of rb+ to r+b so that this works now.

@lobis
Copy link
Contributor Author

lobis commented Nov 13, 2023

@martindurant after starting this PR I realised that there are different places where the mode is specified (not everything goes through the OpenFile.enter) (This is why the new test fails). I could normalize the mode everywhere but it's a bit messy.

Do you agree on the need to normalize (sort) the file mode string? I think it's dangerous to do string comparisons inside the file system against some reference modes ("wb", "rb+", etc.) when these modes can be in different order (but meaning the same thing). Currently I think the only failing case is when using "+" but still I think it should be addressed (memory filesystem cannot be opened with "rb+" or "r+b" because of the reordering taking place).

@lobis lobis marked this pull request as ready for review November 13, 2023 21:01
@martindurant
Copy link
Member

Maybe you can sprinkle set() or sorted() where exact equality is tried? In most cases, in is used instead.

@lobis
Copy link
Contributor Author

lobis commented Nov 13, 2023

At the end I chose not to modify how mode is processed: mode = self.mode.replace("t", "").replace("b", "") + "b".

Instead I replaced all instances of rb+ to r+b. Now it works fine.

I think it makes sense anyway that the file mode should always have b at the end since this is a python thing.

@lobis lobis changed the title Normalize string mode when opening file Fix inconsistent file mode formatting and comparison (rb+ changed to r+b) Nov 13, 2023
@lobis
Copy link
Contributor Author

lobis commented Nov 13, 2023

@martindurant this is now ready, please let me know if you any have feedback.

@martindurant
Copy link
Member

python seems to agree

@martindurant martindurant merged commit 70be2bf into fsspec:master Nov 14, 2023
10 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.

2 participants