-
Notifications
You must be signed in to change notification settings - Fork 11
Replace typer with click #39
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
Conversation
evenharder
left a comment
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.
Thank you for your constructive work! I have also previously tried to convert it in my local, and the results are fairly similar. There are some minor comments I want to address.
| base_path, | ||
| wheel_url, | ||
| ): | ||
| """Add a set of package wheels to an existing pyodide-lock.json and |
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.
Arguments are not printed by default in click. If we were to provide consistency, we can explicitly add argument info in the docstring by appending
\b
Arguments:
WHEELS: List of paths to wheel files. (required)
or something similar.
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 see. Thanks for the pointer!
| @click.option( | ||
| "--base-path", | ||
| type=click.Path(path_type=Path), | ||
| default=None, |
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.
Is None accepted as click.Path? As typer version worked well, it will do so, but I am slightly concerned about its behavior.
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 think it does. I believe click will not change the None to something else, and it should be fine then.
| assert "needs-one-opt" in example_lock_spec.packages | ||
| assert example_lock_spec.packages["needs-one-opt"].file_name.startswith( | ||
| "http://www.nowhere.org/dist/nEEds" | ||
| "http://www.nowhere.org/dist/needs" |
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 wonder why it was written with nEEds in the first place, it should have been intentional?
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.
probably to test name canonicalization
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 am not sure why the original test was written with nEEds and why it passed. The filename should be canonicalized I gueess.
|
Love removing deps, but why not go all the way and use |
Well, we would like to integrate different tools in a single |
Thanks. I hope my work didn't conflict with yours. I was trying to improve our build system and wanted to push this forward. If you are working on other packages, please let me know. |
Drops typer dependency
cc: @evenharder