-
Notifications
You must be signed in to change notification settings - Fork 52
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 as wrapper to core functionality #73
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.
This is great, thanks @ivergara! 🎉
I've got a few requested changes and some tweaks while you are in these files that would be great to have.
deon/cli.py
Outdated
@@ -28,41 +21,20 @@ | |||
Default is False , which will append \ | |||
to existing file.') | |||
def main(checklist, format, output, clipboard, overwrite): |
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.
While you're in here, could you rename format
to output_format
so we don't clash with built-in the python function.
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.
Actually, looks useful for both cli.py
and deon.py
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.
That's a good idea, thought about that while refactoring but didn't want to take that liberty.
deon/deon.py
Outdated
pass | ||
|
||
|
||
def create(checklist, format, output, clipboard, overwrite): |
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.
Let's add tests that directly hit this entry point.
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'd suggests to re purpose the previous tests that were testing the CLI and expand them, and then simplify the CLI testing.
While at the topic of testing, should I take the task to activate code coverage report within the test runner?
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.
Yep, that approach works fine. I'll add codecov to CI so we can get those reports per PR. For some reason, CI stopped running on this repo, and we're working on fixing that.
@@ -0,0 +1,53 @@ | |||
import os |
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.
Do we need to add something like from deon import create
to __init__.py
so that we can import deon.create
instead of deon.deon.create
?
deon/deon.py
Outdated
if ext in EXTENSIONS.keys(): | ||
output_format = EXTENSIONS[ext] | ||
else: | ||
raise ExtensionException |
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.
Can we pass a message that includes the ext
var so it is easier to debug?
deon/deon.py
Outdated
if format in FORMATS: | ||
output_format = format | ||
else: | ||
raise FormatException |
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.
Can we pass a message that includes the format
var so it is easier to debug?
click.echo(template.render()) | ||
# write output or print to stdout | ||
if result: | ||
click.echo(result) |
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.
Can we add a click.echo
call for clipboard and writing to a file that tell the user what happened and that it succeeded?
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.
In this I didn't understand exactly what you mean. Perhaps you could rephrase it?
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.
Something like:
elif clipboard:
click.echo("Checklist successfully copied to clipboard.")
else:
click.echo(f"Checklist successfully written to file {output}.")
Thanks for the feedback! I didn't want to change much in the first iteration, going to the core of the issue that prompted this PR. I'll go through the comments answering and replying. |
Hi, I did fix most of the comments. There is just the temporal issue that the tests of the |
Awesome, thanks @ivergara! Would you mind just pulling the |
Oops (it was late for me). There it is. I'll work on improving the tests, as in removing the high amount of duplicity and trying to not only have a high coverage but also testing combinations of arguments. |
Thanks @ivergara! |
Also, just to note, I had to clean up flake8 and some failing builds after merging these changes. You can run See this commit for the changes: |
As commented in issue #66, I did separate the logic into a
create
function thinning out the CLI. It is basically a reorganization of the code with minimal changes beyond adding very basic customExceptions
.