-
Notifications
You must be signed in to change notification settings - Fork 6
Release 0.12.8 #144
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
Release 0.12.8 #144
Conversation
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.
Pull Request Overview
This release bumps the version to 0.12.8, reorganizes Looper and pipeline interface templates, and enhances the CLI and requirements for automated SRA conversion.
- Bump package version and include
--versionin both CLI andsraconvertcommands - Move pipeline and Looper YAML templates into
geofetch/templatesand update path resolution - Add (p)ypiper requirement and upgrade CI actions
Reviewed Changes
Copilot reviewed 15 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| requirements/requirements-all.txt | Added new dependency for automated SRA conversion |
| pipeline_interface_convert_v1.yaml & pipeline_interface_convert.yaml | Removed legacy pipeline interface definitions |
| manual_testing.py | Added a simple example script for manual testing |
| geofetch/templates/* | Introduced new Looper and pipeline interface templates |
| geofetch/sraconvert.py | Switched to VersionInHelpParser and added a --version flag |
| geofetch/const.py | Defined TEMPLATES_DIR and new template name constants |
| geofetch/geofetch.py | Updated all template file lookups to use TEMPLATES_DIR |
| MANIFEST.in | Updated to include everything under geofetch/templates/ |
| .looper.yaml | Added an example local Looper config file |
| .github/workflows/black.yml | Upgraded actions/checkout and actions/setup-python versions |
| geofetch/cli.py | Switched main CLI to VersionInHelpParser and embedded version |
| geofetch/_version.py | Bumped __version__ to 0.12.8 |
Comments suppressed due to low confidence (5)
requirements/requirements-all.txt:10
- The project code imports
pypiper, but the requirement is listed aspiper. It should bepypiper>=0.14.4to match the actual package name.
piper>=0.14.4
geofetch/const.py:52
- [nitpick] The constant
LOOPER_SRA_CONVERTsuggests an SRA conversion template but holds the Looper config template filename. Consider renaming toLOOPER_CONFIG_TEMPLATE_NAMEfor clarity.
LOOPER_SRA_CONVERT = "looper_config_template.yaml"
.looper.yaml:4
- [nitpick] This example config references
pipeline_interface_convert.yamlat the repo root, but that file has been removed. You may want to point to the template undergeofetch/templatesor remove this tracked example.
- /home/bnt4me/virginia/repos/geofetch/pipeline_interface_convert.yaml
manual_testing.py:1
- [nitpick] This example script lacks a module docstring or comments explaining its purpose and usage. Consider adding a top-level docstring to clarify how to run and what it demonstrates.
from geofetch import Geofetcher
geofetch/sraconvert.py:75
- This version flag references
__version__but__version__is not imported in this module, which will raise aNameError. Import it fromgeofetch._versionor adjust accordingly.
parser.add_argument(
donaldcampbelljr
left a comment
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 great!
| """ | ||
| _LOGGER.info("Expanding metadata list...") | ||
| list_of_keys = _get_list_of_keys(metadata_list) | ||
| for key_in_list in list_of_keys: |
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.
This is minor, but I'd recommend simplifying variable names like this to key instead of key_in_list. Given the logic, it is implied, and so, the name is perhaps a bit redundant.
| import os | ||
| import sys | ||
| from argparse import ArgumentParser | ||
| from ubiquerg import VersionInHelpParser |
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.
Why did we change from using ArgumentParser to a class from ubiquerg?
Changes: