Skip to content

MAINT: refactor config parsing and validation code #334

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

Closed
wants to merge 2 commits into from

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Mar 7, 2023

This provides a major of the config handling code, making it much easier to add new config options.

FFY00 added 2 commits March 7, 2023 01:23
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00
Copy link
Member Author

FFY00 commented Mar 7, 2023

It also now reports all errors for the hook config_settings, or the pyproject.toml file together, instead of failing one by one.

@FFY00 FFY00 added the enhancement New feature or request label Mar 7, 2023
@dnicolodi
Copy link
Member

@FFY00 There is a pull request touching the same exact code and refactoring it in an IMO elegant and effective way in 30 line of code (compared to the 200 lines of code of this PR). That PR is open since more than a month and have received no review and has an open question to you since a few weeks, without response. There are other PRs that are awaiting reviews. Now you come up with this: 200 lines of over engineered code to check the shape of a dict in which it is absolutely unclear what are the allowed keys and values. The impression is that you don't want to review code written by others to be included in "your" project. In other circumstances I would simply walk away from a project where the appointed maintainer seems too proud to review contributions. However, you also go around criticizing the maintainers of other projects for not being collaborative. I just want to make you think about it.

FWIW: NAK base that these are more than 200 lines of code without any test.

@FFY00
Copy link
Member Author

FFY00 commented Mar 7, 2023

Hi @dnicolodi, I am sorry I made you feel this way, it was not my intention.

My goal with this PR was to find out what was required to move the unknown keys checks to pyproject-metadata, which I eventually want to move there. This probably isn't great, this kind of development should probably go into that repo directly.

There is a pull request touching the same exact code and refactoring it in an IMO elegant and effective way in 30 line of code (compared to the 200 lines of code of this PR). That PR is open since more than a month and have received no review and has an open question to you since a few weeks, without response.

I did comment on that PR, however, it must have fell off my mind, and last week when I went through all open PRs it didn't catch my attention because the discussion was marked as resolved (I have now unresolved it).
Unless it's a major issue, and I want to block the PR, I avoid using Github's "request changes" button because I find its language a bit out of place. I meant my interaction on that PR as a review, but I understand that wasn't very clear.
Which procedure would you like me to adopt when reviewing your PRs?

There are other PRs that are awaiting reviews. Now you come up with this: 200 lines of over engineered code to check the shape of a dict in which it is absolutely unclear what are the allowed keys and values.

A lot of complexity is due to the validation being based on the dataclass fields, which I plan to make more declarative, but I just need to sort out some details. This also makes it so that pyproject.toml gets the unknown keys checks.

But like I said above, this should probably happen in pyproject-metadata directlly instead.

The impression is that you don't want to review code written by others to be included in "your" project. In other circumstances I would simply walk away from a project where the appointed maintainer seems too proud to review contributions.

I'm sorry I made you feel this way. I will more careful and try to avoid situations that could be read as that in the future.

FWIW: NAK base that these are more than 200 lines of code without any test.

We already have tests for this, and they pass, I even had to change them to reflect the new error messages, so I don't follow here.

@dnicolodi
Copy link
Member

You comment on #304 but you didn't either approve or request changes but left a comment suggesting some more refactoring. My request for clarification has been left unanswered for weeks till I finally resolved the conversation. What should I have assumed that means?

A lot of complexity is due to the validation being based on the dataclass fields

That may be a valid design if you are writing a scheme validation library (and even then I have my reserves) but that kind of complexity is completely out of place in this project where we can do the same in a more declarative and infinitely simpler way.

@FFY00
Copy link
Member Author

FFY00 commented Mar 7, 2023

You comment on #304 but you didn't either approve or request changes but left a comment suggesting some more refactoring. My request for clarification has been left unanswered for weeks till I finally resolved the conversation. What should I have assumed that means?

As I mentioned, I lost track of the issue, I think I still have an unfinished reply open somewhere in on of my tabs. I apologize.

@FFY00
Copy link
Member Author

FFY00 commented Mar 7, 2023

Sometimes I lose focus in the middle of doing something and then forget about it. Feel free to ping me as often as you want in situations like this where you are waiting on me.

This is not an excuse, I need to find a better way to deal with this kind of situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants