Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CLI as wrapper to core functionality #73

Merged
merged 12 commits into from Oct 24, 2018
Merged

CLI as wrapper to core functionality #73

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3ee383a
extract the core to its own function separate from cli
ivergara Oct 23, 2018
d900ff4
improving readability of try block
ivergara Oct 23, 2018
252fc8f
fixing import ergonomics
ivergara Oct 23, 2018
6e69ab1
adding vscode to gitignore
ivergara Oct 23, 2018
66a7d8f
removing format keyword clash
ivergara Oct 23, 2018
54a959e
adding message to exceptions pt1
ivergara Oct 23, 2018
ea87f18
moving fixtures to conftest file for better reuse
ivergara Oct 23, 2018
23cedbc
adding coverage
ivergara Oct 23, 2018
5571e7c
coverage and core test, cleanup
ivergara Oct 23, 2018
2941fdb
more tests
ivergara Oct 23, 2018
58ba1ac
removing Pipfile.lock
ivergara Oct 24, 2018
84727d1
extra printing from cli
ivergara Oct 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 13 additions & 41 deletions deon/ethics_checklist.py → deon/cli.py
View file
Open in desktop
@@ -1,15 +1,8 @@
import click
import os
from pathlib import Path

import xerox
from .deon import ExtensionException, FormatException, create

from .parser import Checklist
from .formats import FORMATS, EXTENSIONS

DEFAULT_CHECKLIST = Path(__file__).parent / 'assets' / 'checklist.yml'

CHECKLIST_FILE = Path(os.environ.get('ETHICS_CHECKLIST', DEFAULT_CHECKLIST))
from .formats import EXTENSIONS


@click.command('deon')
Expand All @@ -28,41 +21,20 @@
Default is False , which will append \
to existing file.')
def main(checklist, format, output, clipboard, overwrite):
Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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.

# load checklist
cl_path = Path(checklist) if checklist else DEFAULT_CHECKLIST
cl = Checklist.read(cl_path)

output = Path(output) if output else None

# output extension is given priority if differing format is included
if output:
# get format by file extension
ext = output.suffix.lower()
if ext in EXTENSIONS.keys():
output_format = EXTENSIONS[ext]
else:
with click.get_current_context() as ctx:
msg = 'Output requires a file name with a supported extension.\n'
try:
result = create(checklist, format, output, clipboard, overwrite)
except ExtensionException:
with click.get_current_context() as ctx:
msg = 'Output requires a file name with a supported extension.\n\n'
raise click.ClickException(msg + ctx.get_help())
elif format:
if format in FORMATS:
output_format = format
else:
with click.get_current_context() as ctx:
msg = "File format is not supported.\n"
except FormatException:
with click.get_current_context() as ctx:
msg = "File format is not supported.\n\n"
raise click.ClickException(msg + ctx.get_help())
else:
output_format = 'markdown'

template = FORMATS[output_format](cl)

# write output or print to stdout
if output:
template.write(output, overwrite=overwrite)
elif clipboard:
xerox.copy(str(template.render()))
else:
click.echo(template.render())
# write output or print to stdout
if result:
click.echo(result)
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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}.")



if __name__ == '__main__':
Expand Down
53 changes: 53 additions & 0 deletions deon/deon.py
View file
Open in desktop
@@ -0,0 +1,53 @@
import os
Copy link
Member

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?

from pathlib import Path

import xerox

from .formats import EXTENSIONS, FORMATS
from .parser import Checklist

DEFAULT_CHECKLIST = Path(__file__).parent / 'assets' / 'checklist.yml'

CHECKLIST_FILE = Path(os.environ.get('ETHICS_CHECKLIST', DEFAULT_CHECKLIST))


class ExtensionException(Exception):
pass


class FormatException(Exception):
pass


def create(checklist, format, output, clipboard, overwrite):
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

# load checklist
cl_path = Path(checklist) if checklist else DEFAULT_CHECKLIST
cl = Checklist.read(cl_path)

output = Path(output) if output else None

# output extension is given priority if differing format is included
if output:
# get format by file extension
ext = output.suffix.lower()
if ext in EXTENSIONS.keys():
output_format = EXTENSIONS[ext]
else:
raise ExtensionException
Copy link
Member

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?

elif format:
if format in FORMATS:
output_format = format
else:
raise FormatException
Copy link
Member

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?

else:
output_format = 'markdown'

template = FORMATS[output_format](cl)

# write output or print to stdout
if output:
template.write(output, overwrite=overwrite)
elif clipboard:
xerox.copy(str(template.render()))
else:
return template.render()
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Expand Up @@ -49,7 +49,7 @@ def load_reqs(path):

entry_points={
'console_scripts': [
'deon=deon.ethics_checklist:main',
'deon=deon.cli:main',
]
},

Expand Down
2 changes: 1 addition & 1 deletion tests/test_cli.py
View file
Open in desktop
Expand Up @@ -5,7 +5,7 @@
import json
import xerox

from deon.ethics_checklist import main
from deon.cli import main
import assets


Expand Down