-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: type error #97
fix: type error #97
Conversation
Reviewer's Guide by SourceryThis pull request fixes a type error by changing the type of the Class diagram showing updated DbDocument modelclassDiagram
class DbDocument {
+RunLog run_log
+Optional[str] task_id
+Optional[str] user_id
+Optional[str] work_dir
+Optional[WesEndpoint] wes_endpoint
}
note for DbDocument "Changed work_dir type from Path to str"
class RunLog {
}
class WesEndpoint {
+str host
}
DbDocument --> RunLog
DbDocument --> WesEndpoint
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @JaeAeich - I've reviewed your changes - here's some feedback:
Overall Comments:
- The WES endpoint URL should be configurable rather than hardcoded. Consider moving this to a configuration file or environment variable.
- Please explain why the work_dir type needed to be changed from Path to str. If there were issues with the Path type, we should understand and possibly fix the root cause.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Can you please make 2 PRs out of this? Especially with changes like changing defaults, they should have good visibility.
@uniqueg maybe lets leave changing the url altogether, lets keep that implicit. |
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.
Looks good, but please make sure the PR description is up to date (check whether the issues are still addressed, check that only the type change is described, make sure that the template is either filled out properly or delete it)
yeah it doesn't I have changes the description. |
IMPORTANT: Please create an issue before filing a pull request! Changes need to be discussed before proceeding. Please read the contribution guidelines.
fix type error caused by mishandling path type.
Details
Please provide enough information so that others can review your pull request. Give a brief summary of the motivation. Refer to the corresponding issue/s with
#XXXX
for more information.Testing
Write the appropriate unit and integration tests, if applicable. Make sure these and all other tests pass.
Documentation
Please document your changes and test cases in the appropriate places, if applicable.
Style
Make sure your changes adhere to the coding/documentation style used throughout the project.
Closing issues
If your changes fix any issue/s, put
closes #XXXX
in your comment to auto-close it/them.Credit
Add your credentials to the list of contributors once your pull request was merged.
Summary by Sourcery
Update the WES endpoint URL and fix a type error in the workflow run document.
Bug Fixes:
work_dir
field by converting thePath
object to a string.Enhancements:
https://csc-wes-noauth.rahtiapp.fi
tohttps://wes-na.cloud.e-infra.cz
to use a working endpoint.