refactor: update parsimonious to 0.10.0

[kkirsche]

Closes: #8715

[kkirsche]

The re-ordering of methods in some files is to have the declaration order match that of the upstream source code to simplify the process for individuals who are doing hands-on, manual validation of changes like this.

[kkirsche]

This re-exports the VisitationError from parsimonious.nodes in the __init__.py, which is itself a re-export from the exceptions file. This is re-exported from the nodes file instead though to match the order the names are loaded in https://github.com/erikrose/parsimonious/blob/master/parsimonious/__init__.py in case they change where this comes from.

EDIT: clarity / grammar

[AlexWaygood]

Thanks for the PR! This is really helpful.

The re-ordering of methods in some files is to have the declaration order match that of the upstream source code to simplify the process for individuals who are doing hands-on, manual validation of changes like this.

I understand your motivation here, and readability of stubs is important. However, in the future, I'd much prefer it if you did rearranging of functions like this in a separate PR, rather than bundling the rearranging of the stub into the same PR as some substantive changes. Doing it in the same PR makes it quite difficult for a reviewer to see what's actually changed here, as opposed to what's just been moved.

[kkirsche]

Thanks for the PR! This is really helpful.

The re-ordering of methods in some files is to have the declaration order match that of the upstream source code to simplify the process for individuals who are doing hands-on, manual validation of changes like this.

I understand your motivation here, and readability of stubs is important. However, in the future, I'd much prefer it if you did rearranging of functions like this in a separate PR, rather than bundling the rearranging of the stub into the same PR as some substantive changes. Doing it in the same PR makes it quite difficult for a reviewer to see what's actually changed here, as opposed to what's just been moved.

That materially changes the work of updating the stubs, as was done in this pull request. So I wouldn't have submitted this pull request unless the code was equivalently structured, as it was re-typed by hand to locate the various inaccuracies and missing methods that this had.

Personally, I would counter and say the stubs should have tooling to verify the definition order and should be validated as matching the order of the upstream source to facilitate manual updates, reviews, etc.

EDIT: grammar, content, additional details

[AlexWaygood]

That materially changes the work of updating the stubs, as was done in this pull request. So I wouldn't have submitted this pull request unless the code was equivalently structured, as it was re-typed by hand to locate the various inaccuracies and missing methods that this had.

Absolutely, I understand that, I'm just saying that it would have been easier for me, the reviewer, had this been split into two PRs: one PR to rearrange the stub, and a second PR to implement the substantive changes 🙂

Personally, I would counter and say the stubs should have tooling to verify the definition order and should be validated as matching the order of the upstream source to facilitate manual updates, reviews, etc.

Sure! Wanna figure out how to implement that?

[kkirsche]

Understood. I wanted to use commit history instead, but you've said in other pull requests when I've included context in the history that you wouldn't use that if I included it, so I only had coarse grained pull requests available to me to do it. I understand the situation though. I simply disagreed that this was better to split as it pushes aspects of the maintenance consideration onto your users by issuing a type update release for definition order, without any type updates. Instead, I felt it was more considerate to the users, given the consistent push by the team to reduce impact on your customers, to impose a the burden here instead, where it is only on a small subset of maintainers, instead of all users of that types package.

I should be able to take a look at that, though will need to check what I'm currently committed to. It's more or less equivalent to the concepts underpinning things like isort and to a lesser extent black. stubsort though could walk the two trees, compare the order, being platform-aware, and then restructure the stub's structure appropriately.

[kkirsche]

Either way, I will try to find a way to better communicate these types of changes. I'm not sure pull requests or commit history are entirely the right answers, but we can try to find a way to be more respectful to both sides' time.

[AlexWaygood]

So I wouldn't have submitted this pull request unless the code was equivalently structured, as it was re-typed by hand to locate the various inaccuracies and missing methods that this had.

Thanks for taking so much time and care on this. In case it's helpful, my usual technique when doing this kind of PR is to

• use the results from stubtest (which we can see in [stubsabot] Bump parsimonious to 0.10.* #8715)
• Look at the diff between the two releases, to see what else changed. E.g. you can see the diff between parsimonious 0.9.0 and 0.10.0 here: erikrose/parsimonious@0.9.0...0.10.0

That way I can pinpoint exactly what changed between the releases, so I don't have to retype all the stubs, and the diff stays clean for easy review.

[AlexWaygood]

I simply disagreed that this was better to split as it pushes aspects of the maintenance consideration onto your users by issuing a type update release for definition order, without any type updates. Instead, I felt it was more considerate to the users, given the consistent push by the team to reduce impact on your customers, to impose a the burden here instead, where it is only on a small subset of maintainers, instead of all users of that types package.

Thank you for putting the users of our stubs front and centre. FWIW, I think most users of our stub packages pin their types-* dependencies fairly loosely, so I doubt they will be particularly inconvenienced by a few releases that don't have many substantive changes in them. But the point is well taken that we should probably be more hesitant to make cosmetic-only changes to third-party stubs than we are with our standard-library stubs.

[kkirsche]

I simply disagreed that this was better to split as it pushes aspects of the maintenance consideration onto your users by issuing a type update release for definition order, without any type updates. Instead, I felt it was more considerate to the users, given the consistent push by the team to reduce impact on your customers, to impose a the burden here instead, where it is only on a small subset of maintainers, instead of all users of that types package.

Thank you for putting the users of our stubs front and centre. FWIW, I think most users of our stub packages pin their types-* dependencies fairly loosely, so I doubt they will be particularly inconvenienced by a few releases that don't have many substantive changes in them. But the point is well taken that we should probably be more hesitant to make cosmetic-only changes to third-party stubs than we are with our standard-library stubs.

I hope this doesn't come off wrong, but I explicitly avoided doing that because I don't trust the thoroughness of the tooling. Many of the stubs I've interacted with have legacy changes like Any instead of Incomplete, have allowlist entries, etc. which I generally disagree with. So because the tooling has been configured by the team to be, in my opinion, rather relaxed, I don't have confidence in relying on the tooling for that type of task.

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[AlexWaygood]

I explicitly avoided doing that [...]

I assume you meant to quote my other message. Your message appears to make little sense as a response to the message that you quoted.

I don't trust the thoroughness of the tooling

I'm sorry to hear that. I'm not entirely convinced that your method produces more accurate results, however, if it leads to PRs that propose introducing inaccuracies into the stubs such as import BadGrammar as BadGrammer.

I hope this doesn't come off wrong, but [...] Many of the stubs I've interacted with have legacy changes like Any instead of Incomplete, have allowlist entries, etc. which I generally disagree with. So because the tooling has been configured by the team to be, in my opinion, rather relaxed, I don't have confidence in relying on the tooling for that type of task.

The complaints are getting somewhat tiresome. We know some of our stubs are inaccurate and incomplete. We know that our tooling could be improved. We are trying to improve the situation.

Here are some of the PRs I, personally, have filed to try to improve the situation here.

• stubtest: error for pos-only differences for dunder methods, too mypy#12184
• stubtest: error if a function is async at runtime but not in the stub (and vice versa) mypy#12212
• stubtest: error if a dunder method is missing from a stub mypy#12203
• stubtest: error if an attribute is a read-only property at runtime, but isn't read-only in the stub mypy#12291
• stubtest: emit error if a stub defines a public type alias that doesn't exist at runtime mypy#12608
• Stubtest: verify the contents of __all__ in a stub mypy#12214
• Detect __eq__ and __ne__ methods that use Any instead of object PyCQA/flake8-pyi#163
• Add a check for methods that hardcode their return type, but shouldn't PyCQA/flake8-pyi#174
• Add a check for the proper definition of __all__ PyCQA/flake8-pyi#176
• Introduce Y045: Ban returning (Async)Iterable from __(a)iter__ methods PyCQA/flake8-pyi#248

Further PRs to help improve our tooling are always welcome.

[AlexWaygood]

Thanks for the fixes @kkirsche

[kkirsche]

The complaints are getting somewhat tiresome. We know some of our stubs are inaccurate and incomplete. We know that our tooling could be improved. We are trying to improve the situation.

Understood. Sorry that it is. Each complaint comes off as a response to your feedback ultimately. Do you prefer not responding to your feedback, simply not contributing, or overlooking that? It's all well and good to say this, but what do you actually want done?

Thanks for the fixes @kkirsche

Doesn't really feel like it, it sounds like this relationship may not be working from your feedback.

[AlexWaygood]

sounds like this relationship may not be working from your feedback.

I am always grateful for helpful contributions to this project. Any frustrations I have with this relationship have no bearing on that.

[kkirsche]

sounds like this relationship may not be working from your feedback.

I am always grateful for helpful contributions to this project. Any frustrations I have with this relationship have no bearing on that.

From a contributors perspective, you appear to be the primary maintainer based on actions taken and past history on this repo. As a result, your interactions set the tone for interacting with the entire typeshed team, and how they are perceived by contributors under this org.

With that being the case from my perspective, it sounds like it may make sense for me to take a break and work with a different organization for awhile. I feel this might make sense because it feels like you don't appreciate where I'm coming from with that feedback or my approach to providing feedback.

In addition it comes off as though you feel (from my point of view, not trying to put words into your mouth) that I lack empathy you expect for the burden of being a maintainer. If you disagree or would like to work through the mismatched expectations in a different forum please let me know.

Otherwise I appreciate your time and the time of other team members. It helped me learn more about typing.

[AlexWaygood]

Otherwise I appreciate your time and the time of other team members. It helped me learn more about typing.

Thank you for the many helpful contributions you have made to this project.

[AlexWaygood]

you appear to be the primary maintainer

(FWIW I'm the newest maintainer, have far less experience than most of the other team members, and certainly don't want this title :)

[Akuli]

I don't trust the thoroughness of the tooling. [...] I don't have confidence in relying on the tooling for that type of task.

I agree, and this is why Alex also mentioned looking at the diff of what actually changed. If you also look at the diff and consider how it affects the library, why would it not work?

I also agree that retyping the stubs tends to give good results, but it has downsides too: it makes reviewing large stubs more difficult, and is slower. I see it as suitable for stubs of small libraries, or small and somewhat self-contained parts of bigger libraries.

Any instead of Incomplete, have allowlist entries, etc. which I generally disagree with. So because the tooling has been configured by the team to be, in my opinion, rather relaxed

I don't really see how this is a configuration problem:

• There's no good way to automatically detect which Anys should be changed to Incomplete.
• Ideally each allowlist entry should either have a TODO comment about looking into it, or some explanation about why allowlisting it is the right solution. In fact, Alex went through most of our allowlists manually and fixed various things before he became a maintainer.

Can you get rid of them [blank lines], please?

I disagree with you on this in about as strong of a way as possible, but sure.

I think we should just accept whatever black is happy with. If it allows multiple ways to format the code, we shouldn't complain. The whole point of black is to avoid style nits in reviews.

Each complaint comes off as a response to your feedback ultimately. Do you prefer not responding to your feedback, simply not contributing, or overlooking that?

I think it's good to hear your opinions about our contributing process, tooling, goals etc. Keep letting us know what you think :)

[AlexWaygood]

@Akuli

I think it's good to hear your opinions about our contributing process, tooling, goals etc. Keep letting us know what you think :)

To be clear, I also value feedback on our process. I've found a lot of @kkirsche's feedback in recent weeks and months to be helpful, and I've been trying to do better in response to several of the valid points he's raised.

@kkirsche -- my frustration stems from the fact that it feels like some disagreements are being repeatedly relitigated in every PR at the moment. I feel like it would be more helpful to propose a solution to these problems (preferably, in the form of a PR) rather than restating your position over and over, especially given that some of these problems are pretty longstanding problems that we all agree are problems. Some of your criticisms in this thread also felt needlessly personal ("the tooling has been configured by the team to be, in my opinion, rather relaxed"). Lastly, it feels sometimes like it's difficult to critique any part of your PRs without starting an argument. This response felt needlessly defensive, given that I hadn't even asked you to change anything about your PR, only offered feedback about how you might do better in the future: #8730 (comment).

Anyway, I do value feedback on our process. Perhaps, for now, it would be better if I stepped back from reviewing your PRs, and left that to other members on the team?

@Akuli

I think we should just accept whatever black is happy with. If it allows multiple ways to format the code, we shouldn't complain. The whole point of black is to avoid style nits in reviews.

Fair enough, I was probably too picky here :) In general, I really don't like it when people slip in cosmetic changes into PRs that primarily serve another purpose. But hey.

[lucaswiman]

@kkirsche One change not reflected here from is that .parse can take bytes or str (depending on the grammar). It depends how string literals are defined in the grammar string, so I don't think it would be directly visible to mypy.

It might make sense to make Grammar into a Generic[T] with an appropriate binding on T? Alternatively, we could add specialized subclasses like StrGrammar and BytesGrammar, though that feels a bit heavy just to support annotations.

Separately, is there an argument for keeping this in typeshed rather than annotating the package itself? I'd be happy to review a PR adding annotations to parsimonious itself. The supported versions are only python>=3.7, so there's no longer a reason to avoid them in the code.

[kkirsche]

I'm sorry to hear that. I'm not entirely convinced that your method produces more accurate results, however, if it leads to PRs that propose introducing inaccuracies into the stubs such as import BadGrammar as BadGrammer.

You are correct. I could have, and should have, run the stubs against the inference engine I have to address these. I had been writing stubs by hand to replicate the process python users would most likely use as the stubs tools don't contain inference (not a complaint, just a fact) in order to better understand what other pull requests should be made to this repository or other areas of the ecosystem in order to improve the experience of writing stubs. The fact that typos are easy to introduce is a risk to any type stub writer, and the hope is that by identifying these, they can be addressed for the community rather than simply for me.

@kkirsche -- my frustration stems from the fact that it feels like some disagreements are being repeatedly relitigated in every PR at the moment.

Yes, we do. I don't disagree. The only nuance I'd add is when I ask questions, such as above about what you want, you have repeatedly ignored my questions and instead focused on other things.

I feel like it would be more helpful to propose a solution to these problems (preferably, in the form of a PR) rather than restating your position over and over, especially given that some of these problems are pretty longstanding problems that we all agree are problems.

This situation could have been avoided if instead of critiquing it someone had said, "Hey Kevin, it seems you have some concerns about tooling. While I appreciate the work you are doing on stubs for libraries and the standard library, we don't have the manpower as volunteers to really address some of the issues you are raising at the moment. Could you put together a proposal in an issue for what work you would be able to do to address it so that we can provide feedback before continuing on more stubs? That way we can all be confident no one is wasting their time."

This addresses first the issue of me raising criticisms, provides an opportunity for the team to provide input to the changes before work is done to avoid waste, and sets your expectation of me.

Some of your criticisms in this thread also felt needlessly personal ("the tooling has been configured by the team to be, in my opinion, rather relaxed").

I'm sorry. They were not meant to be personal attacks, but are intended as critiques of ideology or opinions. Ultimately, the team is responsible for the code in the repository, so I'm not sure who else would be responsible for this. In this case, I intended to critique the configuration, the tooling was the subject, and the connection between the two was the team who merged that configuration. My choice of words could have been better in expressing that, but at the same time I'm allowed to have an opinion about your opinions in the same way you are allowed to have them about mine.

Lastly, it feels sometimes like it's difficult to critique any part of your PRs without starting an argument. This response felt needlessly defensive, given that I hadn't even asked you to change anything about your PR, only offered feedback about how you might do better in the future: #8730 (comment).

I can understand that, and apologize for the role I play in this. With that said, I felt it was important to explain to you why I had chosen not to take your advice and why you may not see it used in the future so that we don't have to go through that same loop multiple times.

(FWIW I'm the newest maintainer, have far less experience than most of the other team members, and certainly don't want this title :)

Understood. As one of the more active maintainers at the moment though, you do still come off as the face of typeshed, whether intentional or not.

At the end of the day, I know I can be abrasive and I am working with professionals to address the reactivity you are responding to. I can't fundamentally change my personality over night even if I want to, and not everyone responds to medications to address these types of issues. I've tried extremely hard to critique opinions, decisions, etc. not the people themselves, and I'm sorry that this has not been successful.

As I said before though, if you would like to work through the disagreement, we can. If you instead feel you want to step back from interacting with me, that's ultimately your choice, but I won't continue to contribute to typeshed and will look to avoid other projects you maintain in support of your decision. I don't enjoy hurting peoples feelings or making them mad, and I really do try to avoid it. If the team has a maintainer that I can't interact with, I won't feel as though it's a collaborative environment where I can truly make a meaningful difference in the ways I would like.

At the end of the day, I don't hold any ill will here, but I do feel misunderstood and hurt by the way this played out. As I said, thank you for the time and assistance. I really did learn a lot and appreciated it.

[kkirsche]

@kkirsche One change not reflected here from is that .parse can take bytes or str (depending on the grammar). It depends how string literals are defined in the grammar string, so I don't think it would be directly visible to mypy.

It might make sense to make Grammar into a Generic[T] with an appropriate binding on T? Alternatively, we could add specialized subclasses like StrGrammar and BytesGrammar, though that feels a bit heavy just to support annotations.

Separately, is there an argument for keeping this in typeshed rather than annotating the package itself? I'd be happy to review a PR adding annotations to parsimonious itself. The supported versions are only python>=3.7, so there's no longer a reason to avoid them in the code.

Sorry about that. Based on how the current discussion is going, I'd ask you to make a pull request for this change here if necessary.

I addressed the stubs here simply because they were already here, if Parsimonious is interested in having type hints added directly, I have no issue migrating those over if the library's maintainers are interested. My goal was simply to improve the experience of library users by ensuring they had type information. If you're interested though, I can put that together a pull request with the changes and then a typeshed maintainer can remove the stubs at their discretion.

[AlexWaygood]

when I ask questions, such as above about what you want, you have repeatedly ignored my questions and instead focused on other things.

Okay. I'm trying to answer some of those questions now -- but, that's a fair criticism, and I apologise for that.

As I said before though, if you would like to work through the disagreement, we can.

I've sent you an email with an offer to continue chatting about this offline. I really don't want anybody to feel like they can't contribute to typeshed because of my actions, so I'm keen to work this out. Let me know what works for you.

[AlexWaygood]

@lucaswiman -- if you'd like to incorporate inline type hints into parsimonious itself, that's great! Inline type hints are much less likely to drift out of date than stubs are, so they're often more accurate than stubs.

We have a policy that we generally don't remove stubs from typeshed until the upstream library has had a py.typed file for at least six months -- this is to ensure that upstream types are relatively stable before we drop them from typeshed, and to give downstream users a chance to update their dependencies. But once we get to that point, we'll be very happy to delete them from typeshed.

We're also happy to help out with the process of integrating these stubs into the upstream parsimonious source code, if you run into any difficulties. There's a list of some tools here (written by @kkirsche) that can help automate the process: https://typing.readthedocs.io/en/latest/index.html#type-hint-and-stub-integration

[AlexWaygood]

(For those following along — @kkirsche and I had a zoom call earlier today to discuss some of our miscommunications, and think through some ways we can improve on things in the future. We agreed that we're working towards the same goals, and we're hopeful we can work together better in the future and avoid what happened with this PR. More details to come in the next few days after I've finished writing some things up :)