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

[WIP] Address #36: Allow building-as-an-API #43

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

moshez
Copy link

@moshez moshez commented Jul 8, 2018

Currently this is a minimal refactoring that allows calling an API to build. I've done minimal testing, but mostly I want this out there so we can discuss if this is the approach we want to take, and what else needs to be there. E.g., currently all submodules are not-obviously-private, how do we want to clarify what modules are designed to be imported?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 90

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.6%) to 77.358%

Files with Coverage Reduction New Missed Lines %
.tox/py36/lib/python3.6/site-packages/shiv/cli.py 4 89.86%
Totals Coverage Status
Change from base Build 89: 0.6%
Covered Lines: 205
Relevant Lines: 265

💛 - Coveralls

@lorencarvalho
Copy link
Contributor

Hi @moshez ! Thanks for the PR :)

I tossed some ideas around in this comment on #36 - the key takeaway there is that builder.py is a slightly modified zipapp.py from the stdlib, so perhaps it can be renamed and all the shiv-specific build functions can be placed into a builder.py

I didn't pay much attention to private/public markers in the function/method names because I wasn't thinking of library-esque usage at the time. My hunch is that there's nothing that needs to be marked private. The most important code distinction is that the stuff in bootstrap/ is included in every pyz produced by shiv.

@moshez
Copy link
Author

moshez commented Jul 19, 2018

@sixninetynine Several comments:

  • I think the right approach is the opposite -- only make public what you must, because public APIs are hard to change. If we go with something like my PR, the only public APIs need to be build_output and UserException.
  • I get that bootstrap goes inside the .pyz, but that does not make it a public interface: the only thing calling into it is the __main__.py in the .pyz. I would make it, as well as everything else, private (or do what pip did and move it inside an _internal top-level subpackage.
  • Wherever we decide to actually put the code, it would be nice if __init__.py at the top re-imported it, so it would be available to the user as shiv.build_output. I hope that, aside from exposing the exception class that corresponds to "invalid arguments", we need to make nothing else public.

Let me know!

@moshez
Copy link
Author

moshez commented Jul 26, 2018

@sixninetynine @warsaw any feedback?

@lorencarvalho
Copy link
Contributor

hey @moshez

This diff illustrates what I was thinking: master...sixninetynine:shiv_as_lib

Basically, builder.py becomes zipapp.py, cli.py is split into cli.py and builder.py... The only thing that I'm not that keen on is the func sig on builder.build, it leaves a bunch of work to the user and you lose the validation of arguments that we do in cli.py... what do you think?

@moshez
Copy link
Author

moshez commented Aug 2, 2018

I believe that almost nothing here deserves to be a public module. Do we want people doing from shiv import cli? Or from shiv import zipapp? I don't think we do.

If you agree, we should make all modules either begin with _ or sit under an _internal subpackage, and then just have __init__.py import the public interfaces (of which the only ones should be UserError and create_archive at a first flush).

Base automatically changed from master to main February 11, 2021 06:28
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