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

Split parsing of Poetry versions into a separate module #557

Merged
merged 18 commits into from
Sep 26, 2024

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Sep 6, 2024

Description

While working on conda-lock I noticed that our parsing of pyproject.toml files is incorrect. I'd like to copy it from here. In preparation for this I cleaned up some stuff and split it into a separate module.

When splitting the files I did some fancy Git tricks to preserve the blame history. (That's what the "Split history ..." commits are about.)

Also, as a personal preference, for unit tests regarding the nominal operation of functions I really like doctests. I enabled them. Of course if you don't like that I can revert those commits.

While this PR as a whole may be somewhat confusing, I really try to make each individual commit really easy and quick to understand and verify. I hope I achieved that, but if not, please ask. Thanks a lot for your time!

@maresb maresb requested a review from a team as a code owner September 6, 2024 11:36
@maresb
Copy link
Contributor Author

maresb commented Sep 6, 2024

Ok, the only failing tests now are test_short_license_id, which I'm pretty sure is independent of my changes here.

@maresb
Copy link
Contributor Author

maresb commented Sep 6, 2024

Please have a look at this particular commit: maresb@d367c16

There is an extra whitespace character before sampleSelection, and rgl is missing from the imports, and there is an extra empty string. I don't know enough about R to know whether or not these changes to the expected output are consequential, but since I'm making a real doctest out of them, I want to get an explicit signoff.

@@ -38,7 +38,7 @@ jobs:
- name: Running serial tests
Copy link
Member

@marcelotrevisani marcelotrevisani Sep 20, 2024

Choose a reason for hiding this comment

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

can you revert this and add a new step to run the doctests please?
I think it would be better to have the doctests running step separated from the unittests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, thanks so much for the review!

I can certainly rearrange the tests, but could you explain a bit about the current structure so that I can implement it correctly? In particular, should the doctests execute before or after the serial/parallel tests?

I don't understand why the tests are currently split into serial/parallel, and from what I can see they are executing in exactly the same context, so I'm confused. By default, unless there's some good reason, I'd expect the tests to run all together in the same job, so that if some of the tests fail then you see all the failing tests rather than only the failing tests in the particular job.

Copy link
Member

Choose a reason for hiding this comment

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

The tests are split into serial and parallel because the serial tests can interfere with other tests and make them fail.

@marcelotrevisani
Copy link
Member

that is a nice refactoring, thank you for that!
I just added a comment, if you could change it, it would be nice and after that we can merge it :)

This makes it more self-contained
Please review carefully. The results changed but I know neither R nor the intention.
@maresb maresb force-pushed the copy-py-toml branch 2 times, most recently from 1c2d8ad to 9ded459 Compare September 20, 2024 07:16
@maresb
Copy link
Contributor Author

maresb commented Sep 20, 2024

I backed out of the merge commit and moved the "Enable doctests" commit to be redone to the end.

I will modify it and then redo the merge commit.

At this point there are no changes to the code, only shuffling around commits.

@maresb
Copy link
Contributor Author

maresb commented Sep 20, 2024

@marcelotrevisani, I made an initial guess about what you want. No problem to change it if you'd like it done differently.

@maresb
Copy link
Contributor Author

maresb commented Sep 20, 2024

My previous attempt to split the doctests was broken, but I fixed it and now the CI is green.

@marcelotrevisani
Copy link
Member

Thanks a lot!

@marcelotrevisani marcelotrevisani merged commit d30d42c into conda:main Sep 26, 2024
7 checks passed
@maresb maresb deleted the copy-py-toml branch September 26, 2024 10:12
@maresb
Copy link
Contributor Author

maresb commented Sep 26, 2024

Wonderful, thank you @marcelotrevisani!!!

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.

2 participants