Give file system classes a transaction type, switch to deque #1424
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fsspec/spec.py
Outdated
| @@ -228,6 +228,10 @@ def current(cls): | |||
| return cls._cache[cls._latest] | |||
| return cls() | |||
|
|
|||
| @property | |||
| def transaction_type(self): | |||
| return Transaction | |||
There was a problem hiding this comment.
How about a class attribute? Then it could be overridden either by any subclass having a different attribute or by setting the attribute on any instance.
There was a problem hiding this comment.
Changed, see 0271589.
This means that subclasses of `AbstractFileSystem` need not override the `transaction` property and `start_transaction` function anymore to be able to use custom transaction classes. Instead, they can just override the newly created `transaction_type` property to achieve the same. Also changes the base `Transaction`'s file queue to a `collections.deque` to pick up thread safety on `pop` and `append` events. Finally, returning `self` in the transaction's context entry means that transaction objects can now be access in the enclosing transaction context.
nicholasjng
force-pushed
the
transaction-improvements
branch
from
57266dd to
0271589
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This means that subclasses of
AbstractFileSystemneed not override thetransactionproperty andstart_transactionfunction anymore to be able to use custom transaction classes.Instead, they can just override the newly created
transaction_typeproperty to achieve the same.Also changes the base
Transaction's file queue to acollections.dequeto pick up thread safety onpopandappendevents.Finally, returning
selfin the transaction's context entry means that transaction objects can now be access in the enclosing transaction context.For reviewers:
The
filesmember shouldn't need to be flushed anymore onstart, so this could just be removed?Also, I am not sure on how to make the transaction type an instance variable, as the keyword arguments all seem to go to the
DirCache. (Should I just usekwargs.pop("transaction_type", Transaction)?)Addresses part of #1416.