-
Notifications
You must be signed in to change notification settings - Fork 25
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
Update validate
and versions
, refactor metadata
and urls
#305
Conversation
Reference: #247 Signed-off-by: John M. Horan <[email protected]>
…d code and tests #247 Reference: #247 Signed-off-by: John M. Horan <[email protected]>
Reference: #247 Signed-off-by: John M. Horan <[email protected]>
…247 Reference: #247 Signed-off-by: John M. Horan <[email protected]>
@JonoYang Please review when you have the chance. Looking forward to your suggestions and questions. This comprises my work (subject to input from you and colleagues) on the first four commands: I've substantially updated, refactored and supplemented |
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.
@johnmhoran I have some suggestions for you.
Reference: #305 Reference: #247 Signed-off-by: John M. Horan <[email protected]>
@JonoYang I've addressed all of your comments and committed and pushed; all GH checks passed (I had to rerun a 3.10 push check that failed initially -- transitory validation warning for one of the PURLs in one of the tests). |
@JonoYang I'm taking a closer look at this, which doesn't look quite right. Maybe a copy-paste error on my part? Unable to find the possible source for that approach. This appears twice in
Maybe it should be
Testing manually.... |
@JonoYang Unable so far to manually trigger a |
@JonoYang I've replicated two rounds of the error followed by no error and see the |
Reference: #305 Reference: #247 Signed-off-by: John M. Horan <[email protected]>
@JonoYang All GH checks passed. |
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.
@johnmhoran Looking good, I have a comment about setting the log file location as a constant at the top of the file, under the import statements.
if command_name == "urls" and urls_purl == "not_in_upstream_repo": | ||
warnings.append(f"'{purl}' does not exist in the upstream repo") | ||
continue | ||
log_file = Path(os.path.join(os.path.expanduser("~"), "app.log")) |
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.
I see os.path.join(os.path.expanduser("~"), "app.log")
referenced a few times through the code. I think it would be better to have this as a constant at the top of the file, like LOG_FILE_LOCATION = os.path.join(os.path.expanduser("~"), "app.log")
and then use LOG_FILE_LOCATION
in the necessary places.
I also think the log file should be named purlcli.log
, or something that would indicate the log was created by purlcli
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.
Thank you @JonoYang -- I'll implement these improvements shortly. 👍
Reference: #305 Reference: #247 Signed-off-by: John M. Horan <[email protected]>
@JonoYang Changes implemented, just one local failure with |
Thanks! Merging this now |
Reference: #247