Created scripts/hhs_doi_cli.py and updated dependencies in pyproject.… #11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
quang-ng
reviewed
Nov 29, 2024
scripts/hhs_doi_cli.py
Outdated
| output_exists: bool = output_csv.exists() | ||
|
|
||
| if output_exists and new_run is False: | ||
| print("Removing previously processed PDFs") |
There was a problem hiding this comment.
could we using logger instead of print?
There was a problem hiding this comment.
Done! Set up a package-level logger at dsst_etl.logger
scripts/hhs_doi_cli.py
Outdated
| pdfs = list(pdfs) | ||
|
|
||
| dicts: list[dict] = [] | ||
| with open(output_csv, "a") as f_out: |
There was a problem hiding this comment.
this is too complex, could we break it into smaller functions
There was a problem hiding this comment.
I agree. I broke the main function into 3 and then put all functionout outside of argparse in dsst_etl.hhs_doi.py
scripts/hhs_doi_cli.py
Outdated
| identifier = doi_info.get("identifier") | ||
| identifier_type = doi_info.get("identifier_type") | ||
| extraction_method = doi_info.get("method") | ||
| except Exception: |
There was a problem hiding this comment.
Insert a logger here so we can identify which exception occurs.
Co-authored-by: Quang Nguyen <[email protected]>
leej3
approved these changes
Nov 29, 2024
There was a problem hiding this comment.
I haven't run the code but it looks reasonable to merge. I'm assuming this functionality will be reviewed/used in upcoming development.
I agree with @quang-ngs points:
- Using different logging levels will make things easier to debug in the long run.
- Files in the script directory should have little but a CLI... other functionality should be in the package and ideally it should be tested.
- The function is too large which will harm maintainability. The deep nesting can be avoided by turning the core functionality into its own function.
quang-ng
approved these changes
Dec 2, 2024
…he current working directory.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…toml.
@agt24 modified the filter_cli.py CLI to include DOI extraction with pdf2doi. I further modified it and created this PR. @leej3 , If it doesn't break anything, please approve the review. I'm happy to take any suggestions.