-
Notifications
You must be signed in to change notification settings - Fork 295
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
Backwards-compatible argparse config (not yet in use) #339
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.
Looks good :) I'm kind of surprised that it requires this amount of hacking to be backwards compatible, but it's a nice solution, considering what you had to work with.
When do you plan on integrating this?
|
||
def html_file_arg(arg: t.Optional[str]) -> str: | ||
"""Parse the provided HTML file and return its contents.""" | ||
if arg is None or arg == "pypiserver/welcome.html": |
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.
You may want to create a sentinel, such as _missing=object()
and use that as a default value instead of an (arbitrary string. (Same holds for "pypiserver/no-ignores"
)
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.
The only reason I didn't was because I'm currently using the formatter that automatically adds the defaults to the help text, and I didn't want it to look nasty. However, I think I need to switch to another formatter anyway so that I can get more granular formatting of some of the multiline helps with examples, so I'll make this change too
def html_file_arg(arg: t.Optional[str]) -> str: | ||
"""Parse the provided HTML file and return its contents.""" | ||
if arg is None or arg == "pypiserver/welcome.html": | ||
return pkg_resources.resource_string(__name__, "welcome.html").decode( |
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 all encoding/decoding arguments default to utf-8, so explicitly setting it is not necessary
return pkg_resources.resource_string(__name__, "welcome.html").decode( | ||
"utf-8" | ||
) | ||
with open(arg, "r", encoding="utf-8") as f: |
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.
Have you considered using pathlib? ie Path(arg).read_text()
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 keep forgetting we're in Python3-land now :)
pypiserver/config.py
Outdated
except SystemExit: | ||
# argparse could not parse the arguments. Perhaps they are | ||
# old-style arguments, so we'll try adjusting and re-parsing. | ||
err_msg = ( |
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.
nit: checking for falsity might be enough here? ie err_msg = orig_error or parser.format_usage()
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.
Yeah, normally I'm a stickler for explicit null checks, to avoid unexpected bugs with falsy values, but I think we'd want to use the usage message if this is an empty string anyway, so the falsy check will be good
args.pop(update_idx) | ||
args.insert(insertion_idx, "update") | ||
|
||
return args |
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.
What happens when someone calls pypiserver run -U
?
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.
argparse would throw, because it parses the run
subcommand first, and recognizes that -U
is not a valid argument for that subcommand.
Adding this in to the bottom of this file:
print(Config.from_args(["run", "-U"]))
I get:
usage: config.py [-h] {run,update} ...
config.py: error: unrecognized arguments: -U
Most of the hackiness is around trying to display nice messages, tbh. I'm going to spend a bit of time over the next few days cleaning this up a bit more, and then I'll get it merged.
Trying to get this slotted in to replace the current config will be the next thing I'm working on... the activity in the modernization PR has given me a bit more motivation to work on this project than usual, so I'm hoping I can get that done this upcoming weekend :) |
Thanks very much for the review, by the way! |
|
||
def hash_algo_arg(arg: str) -> t.Callable: | ||
"""Parse a hash algorithm from the string.""" | ||
if arg not in hashlib.algorithms_available: |
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.
Question here: it looks like disabling the hash_algo doesnt work anymore, or am I missing something?
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.
Ah, thanks for catching this. I forgot to handle the "falsey" cases. I'll add that in.
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.
Added in #347
This PR is a pretty substantial refactor of the entrypoints of pypiserver (`__main__` and `__init__`) to use the argparse-based config added in #339. - Updated `RunConfig` and `UpdateConfig` classes to have exclusive init kwargs, instead of taking an namespace. This turned out to be much easier when working with the library-style app initialization in `__init__`, both for direct instantiation and via paste config - Added an `iter_packages()` method to the `RunConfig` to iterate over packages specified by the configuration (note @elfjes, I think that replacing this with e.g. a `backend` reference will be a nice way to tie in #348) - Added a general-purpose method to map legacy keyword arguments to the `app()` and `paste_app_factory()` functions to updated forms - Refactored the `paste_app_factory()` to not mutate the incoming dictionary - Removed all argument-parsing and config-related code from `__main__` and `core` - Moved `_logwrite` from `__init__` to `__main__`, since that was the only place it was being used after the updates to `core` - Updated `digest_file` to use `hashlib.new(algo)` instead of `getattr(hashlib, algo)`, because the former supports more algorithms - Updated `setup.py` to, instead of calling `eval()` on the entirety of `__init__`, to instead just evaluate the line that defines the version - Assigned the config to a `._pypiserver_config` attribute on the `Bottle` instance to reduce hacky test workarounds - Fixed the tox config, which I broke in #339 * Config: add auth & absolute path resolution * Config: check pkg dirs on config creation * Instantiate config with kwargs, not namespace * WIP: still pulling the threads * Init seems to be working * tests passing locally, still need to update cache * Fix tox command * unused import * Fix typing * Be more selective in exec() in setup.py * Require accurate casing for hash algos * Remove old comment * Comments, minor updates and simplifications * move _logwrite to a more reasonable place * Update config to work with cache * Type cachemanager listdir in core * Update config module docstring, rename method * Add more comments re: paste config * Add comments to main, remove unneded check * Remove commented code * Use {posargs} instead of [] for clarity in tox * Add dupe check for kwarg updater * Remove unused references on app instance * Fix typo * Remove redundancy in log level parsing
Adds an argparse config that, while adding subcommands (
pypi-server run
andpypi-server update
), retains full commandline backwards compatibility with the existing config parsing logic.There's a bit of hackery required to do this, so this also issues a warning if using the non-subcommand arguments, allowing us to potentially remove support for the old form in our next next major version bump (i.e. 3.0).
Also adds a
.pyproject.toml
with a black config, and a mypy config block tosetup.cfg
.mypy
is now called intox
, currently only forconfig.py
, because nothing else typechecks successfully.