Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move package to a top-level "src" directory #293

Closed
wants to merge 1 commit into from
Closed

Move package to a top-level "src" directory #293

wants to merge 1 commit into from

Conversation

jdufresne
Copy link
Contributor

This helps ensure that tox tests the colorama installed to the
virtualenv and not otherwise working due to the current working
directory.

The tests directory was moved out of the source directory to the top
level. Test files don't need to be included in the installed package.
They exists for developers, not end users. They still ship with the
sdist so downstream packages can run the tests when re-packaging
Colorama.

The demos directory has been updated to work with the new directory
structure. Its use is now documented in README.rst.

Fixes #278

@tartley
Copy link
Owner

tartley commented Oct 7, 2021

Thanks heaps for submitting this. I agree that this should happen. Apologies I've been AWOL for ages, but I aim to merge some things and make a release soon, and my gut feeling is that this should be included.

@@ -41,6 +41,7 @@ def get_version(path):
url='https://github.com/tartley/colorama',
license='BSD',
packages=[NAME],
package_dir={"": "src"},
Copy link
Owner

@tartley tartley Oct 7, 2021

Choose a reason for hiding this comment

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

update Ignore this comment after all. See thoughts in the next comment:

Could I ask you to also fix line 43 for me, to resemble the one shown in Hynek's article? ie. from:

packages=[NAME],

to something like:

packages=find_packages(where='src')

where 'find_packages' should be imported from setuptools.

Yeesh. Look at that, we're alternatively trying to import distutils, too. We should perhaps drop that. You leave the distutils import alone, I'll go read about whether I can delete it. Sound good? Thoughts welcome.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe my suggestion is a bad idea after all. I'm terrified of breaking things for some project out there that is using distutils - if it turns out to be a high profile one, I'll get a tsunamic of issues, emails, tweets. Requires more thought. Ignore my suggestion above.

@@ -1,2 +1,3 @@
include LICENSE.txt CHANGELOG.rst
recursive-include demos *.py *.bat *.sh
recursive-include tests *.py
Copy link
Owner

@tartley tartley Oct 7, 2021

Choose a reason for hiding this comment

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

Thinking out loud. Do we need to include the tests in the package? I think https://hynek.me/articles/testing-packaging/ suggests we do not.

Could the test runner just import the tests from something like PROJECTROOT/tests ? (which will contain an __init__.py).

I might try it out myself tomorrow, but feel free to try it yourself if this makes sense to you.

@tartley
Copy link
Owner

tartley commented Oct 7, 2021

Also, something in Travis failed for python2.7 and pypy. We'll need to figure that out before this can merge. Hugs!

@tartley tartley added the 0.4.5 Get merged for 0.4.5 release label Oct 7, 2021
@tartley
Copy link
Owner

tartley commented Oct 7, 2021

Some people really DO want tests included in sdists: #183

@tartley
Copy link
Owner

tartley commented Oct 7, 2021

On reflection, I think I still have enough questions about this change that I'm not going to hold up the imminent release for it. But I'm still keen to get this merged when we can, and will make another release for it when ready.

@tartley tartley added requires-thought and removed 0.4.5 Get merged for 0.4.5 release labels Oct 7, 2021
@jdufresne
Copy link
Contributor Author

The pytest CI failure is resolved by #325. I've rebased this on that work.

This helps ensure that tox tests the colorama installed to the
virtualenv and not otherwise working due to the current working
directory.

The tests directory was moved out of the source directory to the top
level. Test files don't need to be included in the installed package.
They exists for developers, not end users. They still ship with the
sdist so downstream packages can run the tests when re-packaging
Colorama.

The demos directory has been updated to work with the new directory
structure. Its use is now documented in README.rst.

Switch test runner to pytest in tox.ini. pytest has been moved out of
the lint_python GitHub workflow as it is now covered by the test
workflow.

Fixes #278
@jdufresne
Copy link
Contributor Author

The pytest CI failure is resolved by #325. I've rebased this on that work.

That didn't quite work out, so I've grouped the pytest fix in as part of this PR.

@tartley
Copy link
Owner

tartley commented Jun 14, 2022

This has developed conflicts but I'd like to get it merged, will have a look when I'm back from vacation in July.

@tartley tartley added the 0.4.6 Get merged for 0.4.6. release label Jun 14, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.4.6 Get merged for 0.4.6. release requires-thought
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a top level 'src' directory
2 participants