Skip to content

Cleanup MANIFEST.in a bit#1901

Closed
nicoddemus wants to merge 1 commit intopytest-dev:masterfrom
nicoddemus:cleanup-manifest
Closed

Cleanup MANIFEST.in a bit#1901
nicoddemus wants to merge 1 commit intopytest-dev:masterfrom
nicoddemus:cleanup-manifest

Conversation

@nicoddemus
Copy link
Copy Markdown
Member

@nicoddemus nicoddemus commented Sep 2, 2016 •
edited
Loading

Remove some invalid entries and exclude files which should not go into a sdist

@nicoddemus
Cleanup MANIFEST.in a bit
4863504 
Remove some invalid entries and exclude files which should not
go into an sdist
nicoddemus
nicoddemus reviewed Sep 2, 2016
View reviewed changes
Comment thread MANIFEST.in
include .coveragerc

include plugin-test.sh
include requirements-docs.txt
Copy link
Copy Markdown
Member Author

@nicoddemus nicoddemus Sep 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I wonder if we should remove those scripts as well? They all seem to be part of a release/testing workflow which is no longer true:

  • plugin-test.sh: test a bunch of plugins located in parent directories using current pytest;
  • requirements-docs.txt: we should advertise using tox -e docs instead.
  • runtox.py: runs tox while pointing to @hpk42 index... it probably made sense when py and pytest release were really tied together, but now I don't think it makes sense.

Copy link
Copy Markdown
Member

@The-Compiler The-Compiler Sep 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All I can say is that I never used them 😉

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 2, 2016

Coverage Status

Coverage remained the same at 93.078% when pulling 4863504 on nicoddemus:cleanup-manifest into 919f50a on pytest-dev:master.

The-Compiler
The-Compiler reviewed Sep 2, 2016
View reviewed changes
Comment thread MANIFEST.in
recursive-exclude testing/freeze/~ *
recursive-exclude testing/freeze/build *
recursive-exclude testing/freeze/dist *

Copy link
Copy Markdown
Member

@The-Compiler The-Compiler Sep 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as doing graft testing/.hypothesis etc.?

@The-Compiler
Copy link
Copy Markdown
Member

The-Compiler commented Sep 2, 2016

I think it's a good idea to define what should happen for every file, which is what check-manifest enforces.

Without your PR:

lists of files in version control and sdist do not match!
missing from sdist:
  .github
  .github/ISSUE_TEMPLATE.md
  .github/PULL_REQUEST_TEMPLATE.md

With your PR:

lists of files in version control and sdist do not match!
missing from sdist:
  .coveragerc
  .github
  .github/ISSUE_TEMPLATE.md
  .github/PULL_REQUEST_TEMPLATE.md
  _pytest/impl
  appveyor.yml
suggested MANIFEST.in rules:
  include *.yml
  include .coveragerc

So I think it's a bad idea to remove those lines, and we should probably prune .github too.

@nicoddemus
Copy link
Copy Markdown
Member Author

nicoddemus commented Sep 2, 2016

I agree. We probably should make check-manifest part of linting, what do you think?

@The-Compiler
Copy link
Copy Markdown
Member

The-Compiler commented Sep 2, 2016

I agree! And probably pyroma too while we're at it.

Note that setuptools-scm breaks check-manifest though (or check-manifest breaks with setuptools-scm, whatever phrasing you prefer). We're not using it yet though.

@nicoddemus
Copy link
Copy Markdown
Member Author

nicoddemus commented Sep 2, 2016

@RonnyPfannschmidt any inputs here?

I'm leaning towards declining this PR and opening a proper task for it, since this is growing out of the (small) scope I originally intended. 😁

@nicoddemus nicoddemus mentioned this pull request Sep 2, 2016
Closed
@nicoddemus nicoddemus closed this Sep 2, 2016
@nicoddemus
Copy link
Copy Markdown
Member Author

nicoddemus commented Sep 2, 2016

Created #1907, thanks for the feedback!

@nicoddemus nicoddemus deleted the cleanup-manifest branch September 2, 2016 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nicoddemus @coveralls @The-Compiler