-
Notifications
You must be signed in to change notification settings - Fork 1
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
Main #351
Main #351
Conversation
entelecheia
commented
Mar 24, 2024
•
edited by ghost
Loading
edited by ghost
Mention [stepsize] in a comment if you'd like to report some technical debt. See examples here.
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
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.
Hey @entelecheia - I've reviewed your changes and they look great!
General suggestions:
- Ensure that the expanded environment variables for paths are validated to prevent potential runtime errors.
- Review the newly added docstrings for accuracy and completeness, especially in utility classes where the functionality is complex.
- Consider the impact of removing parameters from method calls, such as the removal of the 'project' parameter in 'HyFI.run_task', to ensure it aligns with the intended design and usage patterns.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -167,12 +182,14 @@ def inititialize( | |||
if project_description: | |||
project_kwargs["project_description"] = project_description | |||
if project_root: | |||
project_kwargs["project_root"] = project_root | |||
project_kwargs["project_root"] = ENVs.expand_posix_vars(project_root) |
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.
suggestion (code_refinement): Consider validating expanded environment variables for project_root
and global_hyfi_root
.
Expanding environment variables is a great feature, but it's important to ensure the expanded paths are valid and accessible to prevent runtime errors.
def __init__(self): | ||
super().__init__() | ||
""" | ||
A class representing a collection of datasets. |
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.
suggestion (code_clarification): Ensure the new docstring for DATASETs
class aligns with the actual functionality and usage.
While the addition of a docstring is commendable, it's crucial to ensure it accurately reflects the class's purpose, especially since the class methods and attributes are not detailed within.