Skip to content

Conversation

@mahmoud
Copy link
Member

@mahmoud mahmoud commented Jul 16, 2017

Adding a MANIFEST.in to include a few files (LICENSE, README, docs). Not sure if there's a python-hyper standard approach to this, so feedback is welcome!

@mahmoud mahmoud requested review from Lukasa and alexwlchan July 16, 2017 22:25
@codecov-io
Copy link

codecov-io commented Jul 16, 2017

Codecov Report

Merging #33 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #33   +/-   ##
======================================
  Coverage    97.6%   97.6%           
======================================
  Files           6       6           
  Lines        1044    1044           
  Branches      120     120           
======================================
  Hits         1019    1019           
  Misses         13      13           
  Partials       12      12

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a510e8d...eea7687. Read the comment docs.

@alexwlchan
Copy link
Contributor

python-hyper “standard”, insofar as there is one, is to include everything from the repo in the sdist that might be useful – see python-hyper/h2#501

(On a phone, so can’t double-check the proposed MANIFEST.in against the repo contents right now.)

@Lukasa
Copy link
Member

Lukasa commented Jul 17, 2017

Yeah, so my strong recommendation would be to add the testing from python-hyper/h2#501 to validate that the Manifest just basically contains the entire git repo. This seems to be what's most popular with downstream repackagers.

However, if you don't want to do that, that's fine. Just let me know and I'll review with that in mind.

@mahmoud
Copy link
Member Author

mahmoud commented Jul 18, 2017

Alright, python-hyper/h2#501 is fully implemented. Added more files, plus the check-manifest in a packaging tox env. Thanks for the advice and review!

@alexwlchan
Copy link
Contributor

Don’t forget to add the new test to CI.

@Lukasa
Copy link
Member

Lukasa commented Jul 18, 2017

Yup, per @alexwlchan's note we should add the new test env to the CI. 😁

Copy link
Member

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Cool, assuming we get a green test run we can go ahead and merge this.

Copy link
Contributor

@alexwlchan alexwlchan left a comment

Choose a reason for hiding this comment

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

🎉

@Lukasa Lukasa merged commit 8090e58 into master Jul 18, 2017
@Lukasa Lukasa deleted the add_manifest branch July 18, 2017 09:38
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.

5 participants