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

Change ScanPoliciesManager interface (see TODOs) #196

Closed
mlech-reef opened this issue Feb 19, 2021 · 7 comments
Closed

Change ScanPoliciesManager interface (see TODOs) #196

mlech-reef opened this issue Feb 19, 2021 · 7 comments
Labels
enhancement New feature or request
Milestone

Comments

@mlech-reef
Copy link
Collaborator

mlech-reef commented Feb 19, 2021

ScanPoliciesManager.should_exclude_file should accept File object, not string
ScanPoliciesManager.should_exclude_directory should accept AbstractFolder object, not string

@mlech-reef mlech-reef added the enhancement New feature or request label Feb 19, 2021
@mlech-reef mlech-reef added this to the ApiVer v2 milestone Feb 19, 2021
@mlech-reef mlech-reef changed the title ScanPoliciesManager.should_exclude_file should accept File object, not string Change ScanPoliciesManager interface (see TODOs) Feb 19, 2021
@mpnowacki-reef
Copy link
Collaborator

@mlech-reef Why is this needed for V2?

@mlech-reef
Copy link
Collaborator Author

It breaks compatibility.

@mpnowacki-reef
Copy link
Collaborator

@ppolewicz In light of

change all path arguments everywhere (internally too) to use pathlib instead of strings

from #139

should should_exclude_file and should_exclude_directory indeed use File and Folder and not pathlib.Path ?

@ppolewicz
Copy link
Collaborator

I think the problem here is that should_exclude_file and should_exclude_directory can have b2:// File / Folder passed into them and pathlib.Path doesn't support b2:// resources

@mpnowacki-reef
Copy link
Collaborator

The way ScanPoliciesManager.should_exclude_file and ScanPoliciesManager.should_exclude_directory work right now is they compare B2 paths to sets of regexes - even though it's not documented, they never operate on local paths. Even when invoked in LocalFolder, a would-be-B2-path of a dir or a file is constructed and passed to the appropriate ``ScanPoliciesManager.should_exclude_*` method. I'm not sure it would be beneficial to create "possibly-existsing-b2-file" objects when scanning local folders just to match their paths against regexes.

@mlech-reef Is this interface change somehow needed elsewhere? Or do you need access to all B2File versions (carried by B2File) in your implementation of ScanPoliciesManager ?

@mpnowacki-reef
Copy link
Collaborator

However, changing the interface to have the following methods:

  • should_exclude_local_file accepting a LocalFile object
  • should_exclude_b2_file accepting a B2File object
  • should_exclude_local_folder accepting a LocalFolder object
  • should_exclude_b2_floder accepting a B2Folder object

and performing the translations internally would make for a clean, extensible API. The problem is that ScanPoliciesManager objects would need access to local_dir and b2_dir to perform translations accurately. Or maybe a subdelegate of ScanPoliciesManager, a policy manager aware of the particular sync context?

@mlech-reef
Copy link
Collaborator Author

@mpnowacki-reef The TODO was created just to not operate on str-like object carrying paths. Just for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants