MycroftAI / adapt

Adapt Intent Parser
Apache License 2.0
710 stars 154 forks source link

Consolidate package requirements #126

Closed krisgesling closed 3 years ago

krisgesling commented 3 years ago

Is your feature request related to a problem? Please describe. Currently the package requirements are defined in both requirements.txt and setup.py.

Describe the solution you'd like We should consolidate these into a single location which also means they can't diverge. Here's one approach: https://github.com/MycroftAI/mycroft-messagebus-client/blob/master/setup.py#L30

clusterfudge commented 3 years ago

I'm not fully aligned with this strategy; I've tried it in the past, and it actually results in some basic issues. The largest is that there is almost always an intentional difference between packaged build/runtime dependencies and test dependencies, and python does not have a better way of breaking these out.

For example, in #125 , we're introducing a dependency on flake8 and pytest for the build process. Those should not be dependencies of the released artifact, but also probably shouldn't only be specified in the github actions build file.

From the last round of "what's the best way to", I concluded two things:

I'm curious though, is this opinion in conflict with #124 ? Theoretically if source builds include test files, you would want to specify their dependencies somehow.

forslund commented 3 years ago

Adapt has the file test-requirements.txt which could be updated and used in the github-actions.

I've recently learned that the -r flag can be used in requirements files to include another file. so the test-requirements.txt could be updated to be

pytest
flake8
-r requirements.txt

(possibly renaming it to dev-requirements.txt or something)

There is also the option of having the setup.py called from the requirements file. Keeping the necessary requirements in the setup.py and dev requirements in requirements.txt.

Then another option that I've been looking at for mycroft-messagebus-client is to use the tests_require and test arguments for setup() to be able to call python setup.py test to perform the tests

clusterfudge commented 3 years ago

-r requirements.txt

oooooooohhhhhhhh! ✨

forslund commented 3 years ago

Closed via #127