190 Add package change confirmation #293
Conversation
lights7412
force-pushed
the
190-add-package-change-confirmation
branch
2 times, most recently
from
5daaab6
to
ee06943
Compare
cli/functionary/package.py
Outdated
| @click.pass_context | ||
| def publish(ctx, path, keep): | ||
| def publish(ctx, path, keep, y): |
There was a problem hiding this comment.
You can give y a name in the @click.option declaration above so that it has a more meaningful variable name here. See the documentation on naming options. Let's name it skip_confirm.
cli/functionary/package.py
Outdated
| if not y and changes: | ||
| confirm = input("Continue [y|N]? ").lower() | ||
| if confirm not in ("y", "yes"): | ||
| exit(1) |
There was a problem hiding this comment.
You'll want to use sys.exit() here. We might already make this mistake elsewhere, but exit() is really for the shell and not meant to be used in program code.
lights7412
force-pushed
the
190-add-package-change-confirmation
branch
5 times, most recently
from
f9b07c7
to
b669097
Compare
There was a problem hiding this comment.
Overall this looks much improved and my remaining comments are pretty minor.
One issue that I failed to test for before is that the check raises an exception when publishing a new package because it assumes it's going to find a package in the API call. You'll need to handle the case where there's no existing package to compare the one being published to.
cli/functionary/utils.py
Outdated
| Sorts functions by their package id | ||
| """ | ||
| functions_lookup = {} | ||
| package_id = 0 |
There was a problem hiding this comment.
This initialization doesn't look necessary based on what happens after it.
cli/functionary/utils.py
Outdated
| packagefunctions = _get_package_functions(path)["functions"] | ||
| apifunctions = _get_functions_for_package(_get_package_functions(path)["name"]) | ||
|
|
||
| # Nothing in the api |
There was a problem hiding this comment.
You can remove this comment. The if not apifunctions is kind of self-explanatory.
cli/functionary/utils.py
Outdated
| if updatedfunctions: | ||
| _format_updated_functions(updatedfunctions, "Updated Functions") | ||
|
|
||
| return newfunctions or removedfunctions or updatedfunctions |
There was a problem hiding this comment.
There's a little bit of ambiguity with what you are returning here. newfunctions or removedfunctions or updatedfunctions will return the first of those values that isn't empty. You then check that value for truthiness later on. What would be more explicit would be to return a boolean here.
lights7412
force-pushed
the
190-add-package-change-confirmation
branch
3 times, most recently
from
5fa524b
to
7137391
Compare
lights7412
force-pushed
the
190-add-package-change-confirmation
branch
from
7137391
to
6c3e635
Compare
Closes #190
Checks if there are changes between the package.yaml and the api when publishing. If there are changes it will display the associated changes and ask if you want to continue.
The confirmation can be skipped by adding a -y to the publish command <functionary package publish ~/functionary/examples/demo -y>.
Three Unit tests were added in the cli/tests/tests_utils.py. Further testing can be done by adjusting the package.yaml (ie adding, deleting, changing functions)