Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

MANIFEST only necessary files for package install #195

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Jul 6, 2023

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

We currently ship the benchmarks and tests with the source distribution. We shouldn't do this.

What does this PR do?

  • Tells the MANIFEST.in to ignore the unnecessary directories.
  • Adds a MANIFEST check to the CI, to bring it in line with other BrainGlobe packages.

References

How has this PR been tested?

Is this a breaking change?

No

Does this PR require an update to the documentation?

No

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@willGraham01 willGraham01 marked this pull request as ready for review July 6, 2023 15:50
@alessandrofelder
Copy link
Member

alessandrofelder commented Jul 7, 2023

LGTM!

For the record, some thinking and discussion with @lauraporta that went into this.

I had second thoughts about removing the tests from the package and read an online discussion (key excerpts below)

My (personal) view is that I want the sdist so that I can build the project on an otherwise-unsupported environment, and I can view the source code of the project (even though pure Python wheels include the source, it’s not in “buildable form”).

and

If the project is non-trivial, you probably need to run the tests to ensure that your build is functional, no?

but given our tests are useless with the the biggish test data in our repo, and we really don't want to ship the test data, let's remove them entirely. Users that want to run the tests on a source install can always clone and install from the repo 🤷

@alessandrofelder alessandrofelder merged commit a8b8992 into main Jul 7, 2023
@alessandrofelder alessandrofelder deleted the manifest-update branch July 7, 2023 15:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exclude tests and benchmarks from distributed package
2 participants