Repository navigation
Misc review bugs - #123
Merged
Merged
Misc review bugs#123
Conversation
The iterator returned by ThreadPoolExecutor.map was discarded, so any exception raised by download_image (an HTTP error, or running out of retries) was never seen. The command reported that every image was downloaded and wrote metadata for images that weren't. Consuming the results raises the first failure. It also means each chunk finishes downloading before the next page of images is fetched, instead of the entire result set being queued up front. Since map cancels its pending futures when the iterator is interrupted, Ctrl-C now only waits for the downloads in progress. Before, it waited for every remaining image to download.
The SIGINT/SIGTERM handler removed every .isic-partial file before exiting, including the ones worker threads were still writing. Those downloads then failed when moving the file into place, so every image in progress when Ctrl-C was pressed was thrown away. On Windows the open files can't be removed at all, which is likely where the "Permission error while cleaning up" warnings came from. Leave the cleanup to the atexit handler, which runs after the worker threads have finished. SIGINT already raises KeyboardInterrupt, and SIGTERM now does the same so that atexit handlers still run.
get_images interpolated the search into the URL without encoding it, so a query containing & was split into separate parameters, and a + was read by the server as a space. get_num_images passes the same search through params= and encodes it correctly, so the reported count and the images actually downloaded could disagree.
An empty --from-isic-ids file meant no requests were made, so the summary had no "succeeded" key and building the results table raised a KeyError, which was reported as a bug in isic-cli. While here, label any status the server adds in the future with its raw name instead of raising a KeyError after the images have already been changed, and show the first three examples in sorted order. Three arbitrary IDs were picked from a set before sorting.
suggest_guest_login and require_login didn't use functools.wraps, so click.pass_obj copied the wrapper's empty docstring and "isic metadata download --help" showed no description. They also dropped the command's return value. image download passed help=, which hid its docstring and the example search queries in it. Move that text into the docstring instead.
Commands like "isic metadata download > out.csv" write their results to stdout, but two things could end up in the output: - The warning about failing to restore a login was printed to stdout. - -v set HTTPConnection.debuglevel, and http.client prints its debug output to stdout. That output also included the Authorization header. Print the warning to stderr, and log requests through urllib3 instead, which goes to stderr. The urllib3 logging -v tried to enable never worked, since requests.packages.urllib3 is an alias of the urllib3 module and its loggers are named "urllib3.*".
The session was opened with a with block in the cli group callback, so it was closed as soon as the callback returned, before any subcommand ran. Subcommands only worked because requests quietly creates new connection pools on a closed session. Register it on the root context instead, which closes it after the subcommand finishes.
SearchString only checked for a 400 caused by an invalid query. Any other error, such as a 401 from an expired token or a 500, passed validation and the command went ahead as if the search were valid.
The file passed to -o was opened without ever being closed, so writing it relied on the file object being garbage collected (and raised a ResourceWarning). When a search matched nothing, no output was written at all. With -o, the file wasn't created, since the writability check removes the file it probes. Write the CSV header regardless, so an empty result is still a valid CSV.
The license files were the only files image download wrote without an encoding, so they used the locale's encoding. On Windows that's usually cp1252, which can't encode every character a license might contain.
upgrade_type compared the major, minor and micro parts separately without checking that the release was actually newer, so going from 2.0.0 to 1.5.0 counted as a minor upgrade. That can happen when running a version that isn't on PyPI yet. Yanked releases were also treated as available, so yanking a release would keep telling users to upgrade to it, or block them outright if it was a new major version. Read releases from PyPI's Index API (PEP 691) instead of the "releases" key of its JSON API, which PyPI has deprecated. If that key were removed, every command would crash, since the version check only handles request errors. The Index API lists files rather than releases, so versions are read from the file names, and a release with no files, which can't be installed, is no longer considered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.