-
Notifications
You must be signed in to change notification settings - Fork 12
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
Cli edit #50
Conversation
hrshdhgd
commented
May 24, 2021
- Documentation for the click options.
@matentzn , All I've done so far is changed docstrings. Any idea why the build would be failing with the error |
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.
Before we merge this I would like:
At least one, better two or three tests per method in here (make TESTDIR configurable):
https://github.com/mapping-commons/sssom-py/blob/cli_edit/tests/tests.sh
Make sure all CLI methods are tested here:
https://github.com/mapping-commons/sssom-py/blob/cli_edit/tests/test_cli.py
I made a bunch of change to the annotations, but we wont make this perfect now; the goal here is to have cli in an ok state that we can iterate over.
sssom/cli.py
Outdated
df = collapse(df) | ||
msdf = from_tsv(input) | ||
# df = parse(input) | ||
df = collapse(msdf.df) | ||
# , priors=list(priors) | ||
rows = dataframe_to_ptable(df) | ||
for row in rows: | ||
logging.info("\t".join(row)) |
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 guess I am responsible for this... what was I thinking. This isn't logging output, it's the primary output
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 my mistake, not yours.. I want to generally be able to stream outputs from all commands in that way; what is the best way to do this then - using print() in conjunction with a stream (either a file output stream or stdout)?
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.
@hrshdhgd I will fix this
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.
for ptable we are now using print, but I want to ensure with the architecture so will review with @cmungall on Monday and merge.
I have committed tests as well. Things that are pending:
|
Alright this has become too massive, sorry about that @cmungall and I will review this on Monday together and merge it in. |
go ahead and merge! |