-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
[16.0][IMP] fs_storage: compute options protocol + refactor test class for re-use #314
Conversation
Before the change, reading the options protocol might not be in sync with the record protocol. This is particularly the case if the record is save in the middle of editing as the value resets, which can be quite confusing.
294f70d
to
c9ccb24
Compare
@lmignon really tiny change but it makes things a bit easier, as while setting options the user might get confused between host/url/endpoint that mean the same thing but have a different key depending on the protocol. |
I've also added a split of the test class to be able to reuse it in other modules (in particular to work on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @len-foss LTGM
It's indeed less confusing for the end user
So I've pushed another commit to help test other module that might depend on |
@@ -1 +1,2 @@ | |||
from . import common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no need to import common here
if os.path.exists(self.temp_dir): | ||
shutil.rmtree(self.temp_dir) | ||
|
||
def _create_file(self, backend: FSStorage, filename: str, filedata: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need this method... are we going to end up w/ all the possible methods of the backend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of the original code that has just been moved. If you look at the individual commits, the only change is 2ad064e
It makes it easier to re-use the backend and to test with 'optimize paths', which works with subfolders.
/ocabot merge patch |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at b9838eb. Thanks a lot for contributing to OCA. ❤️ |
Before the change, reading the options protocol might not be in sync with the record protocol.
This is particularly the case if the record is save in the middle of editing as the value resets, which can be quite confusing.