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: allow src dir #260

Merged
merged 7 commits into from Nov 3, 2019
Merged

WIP: allow src dir #260

merged 7 commits into from Nov 3, 2019

Conversation

@okken
Copy link
Contributor

okken commented Apr 24, 2019

I'd really like to use flit for projects with src directories.

The flit project already has 2 types of projects.

  1. just a module, like foo.py
  2. a package (directory with __init__.py), like foo/__init__.py

This would add a 3rd and 4th.

  1. just a module, but in src, like src/foo.py
  2. a package in src, like src/foo/__init__.py

It has minimal impact on the rest of the code, only touching 2 functions.
All existing tests still pass.

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Apr 24, 2019

Added some tests.

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Apr 24, 2019

Ok. I see that some code coverage targets aren't getting hit.
I can work on those, if required for the PR to move forward. Just let me know.

@okken okken changed the title allow src dir WIP: allow src dir Apr 24, 2019
@theacodes

This comment has been minimized.

Copy link

theacodes commented Apr 24, 2019

My god, we could settle pypa/packaging.python.org#320 finally.

@Juanlu001

This comment has been minimized.

Copy link

Juanlu001 commented Apr 27, 2019

If I understood correctly, @takluyver concern was not the implementation itself, but the possibility of confusing the users by offering too many options before the discussion is settled.

(For further info, pypa/packaging.python.org#320 and especially pypa/packaging.python.org#320)

@pradyunsg

This comment has been minimized.

Copy link

pradyunsg commented Apr 28, 2019

@theacodes, I don't think this changes anything since @takluyver stated that he doesn't want users to have additional knobs when using flit.

#115 (comment)

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Apr 28, 2019

No extra nobs. It works without changing existing functionality and without adding any new options.

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Apr 28, 2019

Regarding confusing users. It is inevitable, given the benefits of using src, that some tool(s) will develop that support src. It will be less confusing to users to have flit support both models than to have to choose between two different tools.

@glyph

This comment has been minimized.

Copy link

glyph commented Aug 3, 2019

@okken Any luck on getting the test coverage up to snuff?

@jugmac00

This comment has been minimized.

Copy link

jugmac00 commented Aug 17, 2019

@okken Haven't you mentioned somewhere that flit already works with src directories? Could you link it here?

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Aug 17, 2019

Flit does not work with src directories. However, I don’t think it needs to. It would be cool if it had this option. But I’ve convinced myself that the major reasons to use src are solvable by running tests from the tests directory. I’ve not written it up, but I discussed it here: https://testandcode.com/80

@maggyero

This comment has been minimized.

Copy link

maggyero commented Sep 9, 2019

A lot of major Python projects have switched to the src layout now:

And a few major Python projects have adopted a non-conventional lib layout:

So while this PR improve the situation, it would be better if we add an optional -s/--source-dir command-line parameter to give users the choice where they put their source code in my opinion. It is the single most important option for the user: "I want to package THIS code", so it should be available. Flit support options that are not essential, such as -f/--ini-file or --python, so I don't think the "too many command-line parameters" argument holds.

Opinionated is fine but not in this case.

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Sep 9, 2019

@glyph, re "> @okken Any luck on getting the test coverage up to snuff?"

I did this PR as a proof of concept.
I was actually very surprised that something as central as flit had such low coverage as it does.
I'm not interested in increasing test coverage for this PR if @takluyver is going to reject it anyway.

@takluyver If this PR was updated to increase the code coverage, would consider merging it?

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Sep 9, 2019

I've previously stated "Flit does not work with src directories. However, I don’t think it needs to."
I've changed my mind on this.
There are reasons other than tox testing to use src structure, and it should be allowed.

@glyph

This comment has been minimized.

Copy link

glyph commented Nov 2, 2019

@takluyver (any word on @okken's question re: coverage?)

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Nov 3, 2019

I've extended the testing to cover all the changes I made.
I'd love to see the changes merged.

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Nov 3, 2019

@okken Any luck on getting the test coverage up to snuff?

yes. coverage is back up to the target

@okken

This comment has been minimized.

Copy link
Contributor Author

okken commented Nov 3, 2019

The test reason for using src has workarounds other than using src.
However, structurally, it's still often nice to have a src directory.
As many projects have either started or switched to src structure, I'm obviously not the only one that thinks it's a good idea.

@pradyunsg

This comment has been minimized.

Copy link

pradyunsg commented Nov 3, 2019

I realize I might've sounded skeptical in my previous comment (#260 (comment)), so clarifying -- I do think this is an improvement that would be good to have in flit.

This is essentially a superset of the functionality that I'd want in flit so I'm very much on board for this.

@takluyver takluyver merged commit ea347ed into takluyver:master Nov 3, 2019
4 checks passed
4 checks passed
codecov/patch 91.66% of diff hit (target 84.88%)
Details
codecov/project 84.97% (+0.08%) compared to 7f1f894
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@takluyver

This comment has been minimized.

Copy link
Owner

takluyver commented Nov 3, 2019

Sorry I've been silent for so long, and thanks for being patient. I'm going to try to get to Flit 2.0 in the next week or so.

@takluyver

This comment has been minimized.

Copy link
Owner

takluyver commented Nov 17, 2019

Flit 2.0rc2 is now out (rc1 went a bit wrong). See the release notes, and please give it a test drive by installing with pip install --pre flit.

Until a final release is out, you'll need to manually adjust pyproject.toml to allow pre-release versions of flit_core for the build backend:

[build-system]
requires = ["flit_core >=2.0rc2,<3"]
build-backend = "flit_core.buildapi"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.