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

refactor: fix the testing src-layout structure and use relative imports #1431

Merged

Conversation

tonyandrewmeyer
Copy link
Contributor

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/ to testing/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 the ops and scenario 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-level ops namespace rather than from individual modules (except for non-exported names).

@benhoyt
Copy link
Collaborator

benhoyt commented Oct 11, 2024

Oh, this is a pity, as it means the code is (even) more nested. I guess it can't be just testing/*.py or scenario/*.py, because they're two different PyPI packages and need their own pyproject.toml?

@tonyandrewmeyer
Copy link
Contributor Author

Oh, this is a pity, as it means the code is (even) more nested. I guess it can't be just testing/*.py or scenario/*.py, because they're two different PyPI packages and need their own pyproject.toml?

We could go to a flat layout, which would remove the src layer (so instead of moving everything to src/scenario it would be renaming src to scenario). My understanding is that src-layout is considered best practice these days, but I don't have strong feelings about it.

Copy link
Contributor

@dimaqq dimaqq left a 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/.
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

.gitignore Outdated Show resolved Hide resolved
@tonyandrewmeyer tonyandrewmeyer merged commit 53f1bb4 into canonical:main Oct 13, 2024
30 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the scenario-imports-and-layout branch October 13, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants