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

Liu 338 #220

Merged
merged 21 commits into from
Mar 2, 2023
Merged

Liu 338 #220

merged 21 commits into from
Mar 2, 2023

Conversation

awicenec
Copy link
Contributor

@awicenec awicenec commented Mar 2, 2023

Pull request covers mainly the new treatment of the filepath and removal of the dirname component parameters. filepath is now covering both, by slightly changing the interpretation. That also required changing a number of the test graphs. In fact the update also made the treatment of check_file_path_exists more consistent, since it is now really required to set it to false if it does not exist, which is the case for most of the files generated during a workflow run.

In addition also an import bug in pyfunc.py has been fixed and the setting of the sleepTime when translating in test mode has also been fixed.

The many changes in xml2palette.py are automatic code formatting changes and can be ignored, also because the xml2palette.py tool is now deprecated and will be removed soon.

@coveralls
Copy link

coveralls commented Mar 2, 2023

Coverage Status

Coverage: 81.896% (+0.04%) from 81.859% when pulling 63faed7 on liu-338 into 5bf5a47 on master.

Copy link
Contributor

@rtobar rtobar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, specially if all tests are pssing. I left a couple of minor comments, but otherwise it looks pretty good I think.

filename = None

if filepath: # if there is anything provided
if filepath.count("/") == 0: # just a name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purely stylistic, but it reads slightly better to the casual reader:

Suggested change
if filepath.count("/") == 0: # just a name
if "/" not in filepath: # just a name

filename = filepath
dirname = self.get_dir(".")
# filepath = self.sanitize_paths(self.filepath)
elif filepath[-1] == "/": # just a directory name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly:

Suggested change
elif filepath[-1] == "/": # just a directory name
elif filepath.endswith("/"): # just a directory name

if dirname is None:
dirname = "."
filename = os.path.expandvars(filename) if filename else None
dirname = self.sanitize_paths(dirname) if dirname else None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dirname is never None because of the if two lines above

@awicenec
Copy link
Contributor Author

awicenec commented Mar 2, 2023

Thanks for the comments, have integrated those changes as well as merged with the validate-graphs PR.

@awicenec awicenec merged commit 24d9f78 into master Mar 2, 2023
@awicenec awicenec deleted the liu-338 branch March 2, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants