-
Notifications
You must be signed in to change notification settings - Fork 121
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
refactor: fix the testing src-layout structure and use relative imports #1431
refactor: fix the testing src-layout structure and use relative imports #1431
Conversation
Oh, this is a pity, as it means the code is (even) more nested. I guess it can't be just |
We could go to a flat layout, which would remove the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting rid of "scenario" all over the source code is a win :)
tox.ini
Outdated
# build a wheel and then use that across the environments, | ||
# similar to what was done in the ops-scenario repository. | ||
./testing | ||
-e ./testing/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./testing/.
looks funky, but I wonder if this notation is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./testing/.
looks funky, but I wonder if this notation is required.
It can't be testing
(because that would look for a package on PyPI called "testing"). ./testing
and testing/.
both work for me, at least with uv pip
. I don't have a strong preference here - would you prefer either of those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, now that -e
is there, testing
would work because the -e
forces it to be a local reference since you can't have an editable PyPI install.
So it could be -e testing
if you prefer that? There's a small risk that someone drops the -e
or similar and causes problems in the future, but I think testing
is a restricted PyPI name anyway, so people can't actually register something bad under it.
Co-authored-by: Dima Tisnek <[email protected]>
d2f6898
to
2cdcefc
Compare
The original commit merging ops-scenario into this repository had an error in the src-layout structure: the
src
folder should contain a folder for each of the importable packages, not have all of the individual modules.This PR moves everything in
testing/src/
totesting/src/scenario/
to fix this. This means that the build tool can find the package without any help, and makes editable installs work correctly, which removes a quirk in the tox dependency installs. It also makes it simpler to work on the project, since theops
andscenario
packages are available in the expected format with the expected name (although src-layout encourages editable installs for development).For the second part of the cleanup, the PR changes the
testing/src/scenario
imports to be relative for everything inside of that location. While adjusting these, most of the imports are adjusted to import from the top-levelops
namespace rather than from individual modules (except for non-exported names).