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

Make wheelfile a stand-alone module to simplify vendoring #318

Closed
wants to merge 2 commits into from

Conversation

FRidh
Copy link

@FRidh FRidh commented Nov 7, 2019

#262 proposes a public API for the wheel package. That's a good thing to reduce duplication and facilitate reuse. I would expect it to be primarily used by build systems such as setuptools, flit, enscons, mesonpep517, ...

However, bootstrapping of build systems that have external dependencies is often problematic (pypa/setuptools#980). The core of this package is the wheelfile module. Let's make it so that others can easily vendor this now stand-alone module.

The error is used when an error occurs anywhere in the wheel package.
The core part of the package is the wheelfile module. Because its needed
there, and to simplify vendoring of wheelfile let's move the definition
to that file. This change won't affect behavior. Additionally, relative
imports are used throughout the cli for consistency.
@FRidh FRidh force-pushed the vendor branch 2 times, most recently from 5cb1a8f to 68858f5 Compare November 7, 2019 11:23
The utils are only used by wheelfile and its not like its a lot of
utility functions either that it warrants a module of its own. This
simplifies vendoring of wheelfile.
@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #318 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
- Coverage   64.89%   64.82%   -0.08%     
==========================================
  Files          13       12       -1     
  Lines         997      995       -2     
==========================================
- Hits          647      645       -2     
  Misses        350      350
Impacted Files Coverage Δ
wheel/cli/convert.py 35.66% <100%> (ø) ⬆️
wheel/cli/pack.py 90% <100%> (-0.33%) ⬇️
wheel/wheelfile.py 100% <100%> (ø) ⬆️
wheel/cli/__init__.py 25.45% <100%> (-1.34%) ⬇️

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 a7510b6...505c046. Read the comment docs.

@agronholm
Copy link
Contributor

Do you have a specific project you want to vendor this module to? Also, wouldn't it be prudent to decide the public API first?

@FRidh
Copy link
Author

FRidh commented Nov 7, 2019

Do you have a specific project you want to vendor this module to?

I am experimenting currently with a PEP 517 back-end that could support multiple systems, such as meson, cmake and scons.

So far I am fine using my fork, so no need to get it in.

Also, wouldn't it be prudent to decide the public API first?

Maybe. See it as a first step. At this point, it is not fixed yet. But this module (wheelfile) is the most important one, although I'll probably reuse pep425tags as well.

@agronholm
Copy link
Contributor

I will probably end up rejecting this due to the major refactoring I'm doing in the publicapi branch.

@agronholm
Copy link
Contributor

Yeah, you will find that the new wheelfile module in the publicapi branch is quite self contained.

@agronholm agronholm closed this Jun 22, 2020
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.

None yet

3 participants