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: Transition to src style #107

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

RobertRosca
Copy link
Member

Talked about this before in the PR moving extra-geom to GitHub workflows (European-XFEL/EXtra-geom#34 (comment)) but I'll summarise it here again for completeness:

There are many very long discussions about the 'best' way to lay out python packages (pypa/packaging.python.org#320 is a pretty big one, which links to other discussions as well), personally I'm a fan of the src layout instead of the 'standard' way of putting the main module in the root of the package directory for a couple of reasons, the main one being how tests execute.

The standard package layout has the issue where your tests will (typically) perform a relative import of the package and execute code from within the current directory, meaning your tests do not actually run on the package as it would be when installed by a user (n.b. these would be the same if you do an editable install with pip install -e but users typically won't be doing that).

I think the intention behind tests is to check that what users install works correctly, which doesn't end up happening when the tests import the package in a different way to how users will be importing it.

In the end this only matters if you have some compiled code, resources, additional py_modules, or run coverage tests for some scenarios, so in the case of EXtra-data it's probably fine. But it does cause issues with coverage reports for nbval tests in extra geom, and having extra geom and extra data have different layouts would be a bit weird.

This PR is WIP since I'm not too sure how to handle one specific aspect: currently tests are in the package as a module, and as part of the lpd_data notebook in the docs we run !python3 -m extra_data.tests.make_examples to create some example datasets. In the src layout, tests are completely split out from the package modules, so this would no longer be possible and make_examples would have to move into extra_data itself.

@takluyver
Copy link
Member

Note you've got a merge conflict here

@RobertRosca
Copy link
Member Author

Actually, just realised that making this change would be inconvenient for everybody with pending PRs, so I'll go through and chase up some of the old ones to try and close them before thinking about merging this.

Extra-geom has fewer open PRs so that's not really an issue there.

@takluyver
Copy link
Member

In the src layout, tests are completely split out from the package modules

They don't have to be - this is an orthogonal decision with another set of arguments for & against it. The main arguments that I remember are that putting tests in the package makes wheels bigger, but makes it easy to test an installation without a source checkout - e.g. pytest --pyargs h5py; some projects also provide module.test() as a Python function.

I've used both styles at times, but I think I'd lean towards tests-in-package for EXtra-data & EXtra-geom.

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.

2 participants