search

ni / nisystemlink-clients-python

Python API for interacting with a SystemLink Server, created and supported by NI.
https://ni.com/systemlink
MIT License
10 stars 20 forks source link

[setup.py] Prevent tests from being included in .whl #13

Closed alexweav closed 4 years ago

alexweav commented 4 years ago

What does this Pull Request accomplish?

Manually open all prior versions of the .whl and notice that it contains a tests module. These files are also installed when pip installing this package. We don't need to distribute tests, and they bloat the package unnecessarily.

Why should this Pull Request be merged?

This change updates the package exclusion rules to properly leave out everything under the tests directory.

In previous versions, there was an exclusion rule for tests, but this only instructs setup.py to exclude everything directly in the tests module, but does not exclude submodules. To exclude a module and its submodules, both mymodule and mymodule.* should be excluded - see these docs.

Examples were already not included because they do not contain an __init__.py. However for future-proofing, I explicitly added examples to the exclusion rules as well, in case an init is ever added.

What testing has been done?

Built the .whl. Verified that tests was no longer present.

alexweav commented 4 years ago

FYI @DavidCurtiss

alexweav commented 4 years ago

Not sure why Black is now failing on a bunch of unrelated files. Perhaps they updated and we don't have a pinned version?

DavidCurtiss commented 4 years ago

Not sure why Black is now failing on a bunch of unrelated files. Perhaps they updated and we don't have a pinned version?

Yep, that's exactly it. I just reproduced by first running "pip install --upgrade black". (black --check passed before upgrading, and failed afterwards. My prior installed version was 19.10b0.)

In all cases, there was a comma at the end of a single-line list (example below), which black now takes to mean that you want it split across lines.

return self._handle_read(path, response3, http_response,)

You should remove the trailing comma in all those cases, and then both versions of black will be happy with it.

spanglerco commented 4 years ago

Should we be pinning versions to avoid issues like this? It seems like upgrading dependencies should be an intentional process with a dedicated PR so it can resolve things like this.

Also, I was introduced to poetry in a recent presentation about Python development as an alternative to tox + pip + twine. Could be something to consider.

alexweav commented 4 years ago

@spanglerco The Python working group has discussed the use of poetry, and will almost certainly end up recommending it, there is a lot of support for it across NI. Once it is officially recommended as the toolchain, we should be looking to migrate projects to be compliant.