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.

Sign up for GitHub

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

Jump to bottom

Refactor testing #24

Merged
merged 2 commits into from
Feb 4, 2024
Merged

Refactor testing #24

merged 2 commits into from
Feb 4, 2024

Conversation

chrysle
Copy link
Contributor

@chrysle chrysle commented Nov 21, 2023 •
edited
Loading

No description provided.

@chrysle chrysle mentioned this pull request Nov 21, 2023
Merged
pradyunsg
pradyunsg approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the changes to the example packages -- the rest LGTM and I'm happy to defer to someone else to review that or for @chrysle to merge this w/o review if that takes too long.

cclauss
cclauss approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Nice!! Thanks.

webknjaz
webknjaz reviewed Nov 21, 2023
View reviewed changes
requirements.txt Outdated Show resolved Hide resolved
@chrysle chrysle mentioned this pull request Nov 27, 2023
Merged
@abravalheri
Copy link
Contributor

abravalheri commented Nov 27, 2023 •
edited
Loading

Hi @chrysle I am not sure about this one...

One of the objectives of the repository is to show people compatibility of mixing and matching namespaces isn't it. If all examples use the src directory, it is more likely everything work fine because all editable installations are performed via .pth, and that is the "best-behaved" installation method ...

I think there is value in the information provided for the flat-layout. We probably should keep that1. And yes this means that the CI (as it is configured right now) is going to fail always.

The noxfile.py as it is currently written is a tool to create the table.md file, and not to run in the CI (at least not all the defined sessions). It is part of its functionality that some nox sessions will fail.

Footnotes

  1. Or maybe we need 2 tables, one for flat-layout and one for src-layout. ↩

@chrysle
Copy link
Contributor Author

chrysle commented Nov 27, 2023

Thank you for reviewing, I understand your reservations and will happily revert this change.

Or maybe we need 2 tables, one for flat layout and one for src-layout.

I'm unsure how to accomplish this, thinking of the directory structure..... Should we duplicate the packages with different layouts?

@abravalheri
Copy link
Contributor

I'm unsure how to accomplish this, thinking of the directory structure..... Should we duplicate the packages with different layouts?

Tricky one!

The not ideal/ad-hoc, but easy-way-out, is to leave the flat-layout and the src-layout in 2 separated branches and maybe add links... It is ugly, but fast to do...

Astormalo

This comment was marked as spam.

Sign in to view

@chrysle
Copy link
Contributor Author

chrysle commented Nov 28, 2023

@abravalheri I've gone ahead and disabled that the status checks are required. I'll set up the branching suggestion soon and move the affected changes in this PR to a src-layout branch. Notifying you once I'm done.

@chrysle chrysle changed the title Refactor testing and migrate examples to src-layout Refactor testing Nov 29, 2023
@chrysle
Copy link
Contributor Author

chrysle commented Dec 14, 2023

@webknjaz Could you review again?

@chrysle
Copy link
Contributor Author

chrysle commented Jan 7, 2024

@webknjaz Gentle ping.

@chrysle chrysle merged commit a4b1224 into master Feb 4, 2024
0 of 6 checks passed
@chrysle chrysle deleted the refactor-nox-workflow branch February 4, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abravalheri abravalheri abravalheri left review comments

@willingc willingc willingc left review comments

@cclauss cclauss cclauss approved these changes

@Astormalo Astormalo Astormalo left review comments

@pradyunsg pradyunsg pradyunsg approved these changes

@webknjaz webknjaz Awaiting requested review from webknjaz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@chrysle @abravalheri @webknjaz @willingc @pradyunsg @cclauss @Astormalo