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

Allow file: for dependencies in TOML #3255

Merged
merged 1 commit into from Jun 19, 2022

Conversation

akx
Copy link
Contributor

@akx akx commented Apr 7, 2022

Summary of changes

This is the pyproject.toml counterpart of #3253, as requested by @abravalheri in #3253 (review).

Pull Request Checklist

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Thank you very much for the very quick implementation @akx.

You went even further than I was expecting and also provided a patch for validate-pyproject, that is greatly appreciated!

I left some review comments. Some of them are minor, but I think the one about _ensure_previously_set is important to implement.

Would you also be OK allowing the maintainers to implement changes in the PR? There are minor things/nitpicks that I would edit before accepting the PR, but those are really not worth bothering you.


BTW: sorry for the trouble of requiring you to change the schemas in a different project. My long term vision is to move them to the setuptools repository.

Very great that you managed to find the way of re-generating the validation code even without proper documentation for tox -e generate-validation-code 👏 ❤️


Regarding the acceptance, let's see how Jason review the other PR for setup.cfg. When that one gets merged, I am very happy to merge the changes here.

@akx akx force-pushed the toml-file-requirements branch 2 times, most recently from edaa15b to 1de7c5a Compare April 8, 2022 08:33
@akx akx requested a review from abravalheri April 8, 2022 08:33
@akx
Copy link
Contributor Author

akx commented Apr 8, 2022

I left some review comments. Some of them are minor, but I think the one about _ensure_previously_set is important to implement.

Addressed, thank you!

Would you also be OK allowing the maintainers to implement changes in the PR? There are minor things/nitpicks that I would edit before accepting the PR, but those are really not worth bothering you.

Honestly I'd prefer if you could make review comments instead, I don't mind doing the work! That'd also give me a bit more insight to the stylistic/... choices for setuptools, for future contributions.

Very great that you managed to find the way of re-generating the validation code even without proper documentation for tox -e generate-validation-code

Oh, there was a tox command..? 😁 I just looked at the NOTICE in the _validate_pyproject subdirectory. Also, fastjsonschema seems very interesting – I might have some work use for it at @valohai...

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Thank you very much @akx for providing the changes.
I left a few other comments regarding the previous error in the CI and the nitpicks.

Should we also include an equivalent note in the docs about this method not being intended for pinning?

@akx
Copy link
Contributor Author

akx commented Apr 8, 2022

@abravalheri Review comments addressed, docs added! :)

@akx akx requested a review from abravalheri April 8, 2022 09:03
Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Thank you very much @akx, it looks very good to me.

I think we can merge this once the other PR is approved.

Any chance you can have a look on the CI to see why is it failing?

@akx
Copy link
Contributor Author

akx commented Apr 8, 2022

@abravalheri The CI error was just a flake8 complaint about a line length. Fixed that, as well as the requirements.txt formatting in the docs.

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.

None yet

2 participants