CS-2323 _aliases: Do not replace non-top-level directories
#34
Conversation
|
This is needed to solve: https://jira.ultimaker.com/browse/CS-2323 |
xli12
requested review from
allardhoeve,
bryanfoley,
ChrisTerBeke,
CoenSchalkwijk,
jellespijker and
rburema
|
I think we need a test that reads a little like this: 👍: /materials/something.txt --> /files/materials/something.txt Maybe this?: def write_package_with_file_ and_read_back(location):
# The function you wrote
return new_path
assert write_pwfarb("/materials/something.txt") == "/files/materials/something.txt"This text you wrote is clearer to me than the test you added before. The code is just really confusing in its intent, sorry. |
|
Added @nallath to make sure the new code does what Cura expects it to do. |
| @@ -298,7 +298,11 @@ def _processAliases(self, virtual_path: str) -> str: | |||
|
|
|||
| # Replace all aliases. | |||
| for regex, replacement in self._aliases.items(): | |||
| virtual_path = re.sub(regex, replacement, virtual_path) | |||
| if regex.startswith("/"): | |||
| expression = r"^" + regex | |||
There was a problem hiding this comment.
| expression = r"^" + regex | |
| expression = rf"^{regex}" |
Double checked this. Order of rf doesn't seem to matter.
There was a problem hiding this comment.
On second thought; now that I have taken a closer look at the code, this is a nice fail-safe but isn't this 'just' a case of an incorrect alias definition?
If you look at libCharon/Charon/filetypes/UltimakerFormatPackage.py, you see that all aliases are neatly prefixed with "^/"...
There was a problem hiding this comment.
Indeed in UltimakerFormatPackage there is a ^/ fixed. There expression = rf"^{regex}" doesn't fit here...
On the other hand for replacement of paths, usually / refers to a top-level directory. It seems possible that when using it, even developers may forget putting ^ at the beginning (and consider the method as merely a regex replacement, not a path handler).
Therefore I'll just keep
if regex.startswith("/"):
expression = r"^" + regexThere was a problem hiding this comment.
My first suggestion still stands ;)
|
@xli12 Looks like you need to fix the mypy violations for libCharon first :-/ |
There was a problem hiding this comment.
Code looks fine by me, maybe implement @CoenSchalkwijk his suggestion for using a context manager. Then again it used in the test, so I have no problem if you decide to let it be.
I don't have time to test it with the Spatial plugin today, maybe @rburema can try that but it could be that we need to wait on Arjen his credentials for testing it.
xli12
dismissed stale reviews from jellespijker and CoenSchalkwijk
via
774dec4
| @@ -298,7 +298,11 @@ def _processAliases(self, virtual_path: str) -> str: | |||
|
|
|||
| # Replace all aliases. | |||
| for regex, replacement in self._aliases.items(): | |||
| virtual_path = re.sub(regex, replacement, virtual_path) | |||
| if regex.startswith("/"): | |||
| expression = r"^" + regex | |||
There was a problem hiding this comment.
My first suggestion still stands ;)
Co-authored-by: Coen Schalkwijk <c.schalkwijk@ultimaker.com>
| @@ -298,7 +298,11 @@ def _processAliases(self, virtual_path: str) -> str: | |||
|
|
|||
| # Replace all aliases. | |||
| for regex, replacement in self._aliases.items(): | |||
| virtual_path = re.sub(regex, replacement, virtual_path) | |||
| if regex.startswith("/"): | |||
|
Note that the reason we originally thought the top-level approach wasn't taken from the get go, is because of the way a zipped package can be made: Do submitters have to zip the containing folder, or zip the folders and files directly (zip-bomb)? But maybe this is not an issue for anything that is a materials package? (Or I haven't looked into libCharon too hard: maybe the top-level is also selected contextually?) |
|
@rburema Yeah the issue we fixed in this PR is to avoid that non-top-level directories being accidentally selected and replaced by a specified alias. |
In OpenPackagingConvention, the
_processAliasesmethod simply does the replacement byre.sub(). But we only want the top-level directory to be replaced, for example:👍:
/materials/something.txt-->/files/materials/something.txt👎:
/something/materials/foobar.txt-->/something/files/materials/foobar.txtThis is the fix.
Note: pathlib is not used here because usually a
Pathobject needs to be actually resolvable. APurePathobject doesn't need to be resolvable, but it doesn't have the method to rename/replace. Thereforere.sub()is still used for the fix.