Skip to content

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 akx force-pushed the toml-file-requirements branch from 1de7c5a to d98528f Compare April 8, 2022 09:02
@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 akx force-pushed the toml-file-requirements branch from d98528f to 3b86141 Compare April 8, 2022 13:13
@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

2 participants