Closed thatch closed 4 years ago
Thank you for this contribution.
Before merging, I would just change a couple of things:
In the same way that import is not a good practice, I think that including .txt and *.md isn't a good practice either, what do you think?
As the setup.py references explicitly README.md and requirements.txt, I would explicitly use those instead.
As you know more about setuptools than me, can you tell me if on line 13 of setup.py, where I generate the man page from the AsciiDoc source, is it the best place to put it or not?
I mean, when building a wheel from the sdist package, is the setup.py file used or not? because if it is, then this means that we need the asciidoc toolchain, and also the rnr.1.adoc file in the sdist. On the other hand, if the setup.py file is not used for building the wheel, then we're good as we are now.
In the same way that import is not a good practice, I think that including .txt and *.md isn't a good practice either, what do you think?
I agree with you, if you were to add a test that confirms this works. Otherwise, using a glob is slightly less likely to break if you do have other files in the future (like requirements-dev.txt
or so). I have a few hundred of these to wrangle, so I've left on "allow edits from maintainers" and would recommend you edit to suit.
As you know more about setuptools than me, can you tell me if on line 13 of setup.py, where I generate the man page from the AsciiDoc source, is it the best place to put it or not?
Depends on when you want it to happen. Do you want it included in the sdist too, or only the wheel? I sould recommend looking at https://docs.python.org/3/distutils/extending.html (and some real-world examples in grpc -- https://github.com/grpc/grpc/blob/9facfe2b684bbaaf9e75e2d285e7bffceeefe2b1/src/python/grpcio/commands.py#L107 and the necessary cmdclass
stuff in setup.py
). What you have -- doing stuff at setup.py
import time -- is surprisingly common, and with my practical hat on I think you can leave it if these other options seem overly complicated.
OK, I changed the MANIFEST.in, in order to explicitly add the required files.
By reading the setuptools documentation, it looks like the README.md file is already included by default, without the need for a MAINFEST.in file.
I left the include README.md line in MANIFEST.in, just to be explicit, so that each open() call in setup.py has a matching include line in MANIFEST.in
Include missing files in sdist for #1
Tested with