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.
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
refactor: updates of documentation and extension of unit tests #753
refactor: updates of documentation and extension of unit tests #753
Changes from all commits
99291cb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
not sure if we need this - I would skip since
/
and//
work both, so better to be sure that there is a/
before the file name.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.
mmm I need to test it, because I thought the result of common_path never resulted with a
/
at the end of the path, so i thought that was necessaryThere 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.
After checking, isn't that how the function behave ? and we definitely need to add this
/
?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.
how I understood it (can be wrong), but yes, the result from
common_path()
never ends in a"/"
. so, assume we have"/some/path/some/file.txt"
, then the common path would be"/some/path/some" and assuming the user defines
spectraPath = "/new/path". We would like to replace to get
"/new/path/file.txt". If we always add a
"/"at the
old,
oldwould be in this case
"/some/path/some/"and the
sub()call will replace
"/some/path/some/"with
"/new/path"- and we get
"/new/pathfile.txt". that's why I would not append the trailing
"/"` to the pattern we want to replace.Also,
"/new/path/file.txt"
and"/new/path//file.txt"
both work - so, if the user would providespectraPath = "/new/path/"
(i.e. with a trailing"/"
) it would/should still work.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.
ooooh of course ! No that make complete sense, but ye I think that the less we have
if
statement the better so i will remove it.