-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refine CLI for use with get_table
as almanack <repo path>
#136
Conversation
Co-Authored-By: Gregory Way <[email protected]>
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.
Nice work; seems like you cleaned up a lot of things and added some tests. I made a few comments which probably should be reviewed before merging, but nothing serious.
Co-Authored-By: Gregory Way <[email protected]> Co-Authored-By: Faisal Alquaddoomi <[email protected]>
Co-Authored-By: Faisal Alquaddoomi <[email protected]>
Thanks @falquaddoomi for the review! I've made some changes based on your comments and revisited what @gwaybio mentioned, regarding the unsafe space splitting for the subprocess testing utility function, based on both of your feedback. |
Openly, I wondered if the default behavior of the output returned by |
I think that sounds like a reasonable idea, if you expect people to be using it interactively rather than parsing it. I think it pertains to this PR so it would make sense to include it here, but it's up to you if you'd prefer for it to be reviewed in more detail in a different PR. |
Thanks @falquaddoomi - I created #172 to further this development. I don't know of an exact demand for this right now whereas we can more readily make use of this PR's CLI modifications as is. Separately, @gwaybio , @falquaddoomi , is there anything else I may add to this PR in order to ready it for a merge? Thanks in advance for any further feedback! |
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 don't have anything to add; this all looks good to me!
Thanks @falquaddoomi and @gwaybio for your reviews! Merging this in now. |
Description
This PR refines the CLI to use
get_table
as$ almanack <repo path>
when the Software Gardening Almanack is used throughpython-fire
. The goal was to make using the internal functionality easily from outside of a Pythonic API, for example, as with workflow pipelines that may not use Python API's.Thanks for any feedback!
Closes #134
What is the nature of your change?
Checklist
Please ensure that all boxes are checked before indicating that this pull request is ready for review.