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.

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

Fix extension version #6555

Closed
wants to merge 2 commits into from
Closed

Conversation

noelbundick
Copy link
Contributor

Fixes #6441 by pulling from METADATA instead of metadata.json, which was removed in pypa/wheel#195

PEP 427 defines METADATA, so this change should be safe for all versions of wheel

This allows extension authors to use the latest version of wheel and still have their version / metadata visible in az extension list and az extension show

This is an internal-only change and does not modify any user-facing input/output formats

@codecov-io
Copy link

Codecov Report

Merging #6555 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##           dev   #6555   +/-   ##
===================================
  Coverage    0%      0%           
===================================
  Files       11      11           
  Lines      133     133           
  Branches     9       9           
===================================
  Misses     133     133

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9313117...36228b3. Read the comment docs.

@tjprescott tjprescott requested a review from williexu June 11, 2018 20:54
@derekbekoe
Copy link
Member

derekbekoe commented Jun 11, 2018

@noelbundick The current implementation in the CLI does a json.load which is quick.

This is the full implementation of the method you're using.
https://github.com/jythontools/wheel/blob/master/wheel/metadata.py#L90-L210
This could be very slow to run and extension metadata is needed in several time-critical places in the CLI.

It may be worth checking if this has a perf. impact on the CLI.

@noelbundick
Copy link
Contributor Author

@derekbekoe good catch - yeah it's right at 10x slower. Closing this out for now and may resubmit one in the future if I get time to implement something faster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants