-
Notifications
You must be signed in to change notification settings - Fork 395
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
Incorrect docstring? #811
Comments
You can submit a PR to make this consistent. |
This change will break some code. Looping on iter_choices before: for value, label, checked in field.iter_choices(): now should be: for value, label, checked, render_kw in field.iter_choices(): |
Yeah I think this wasn't a backwards-compatible change and already broke some code: aminalaee/sqladmin#644 I couldn't even see this in the release notes, was that a side-effect? Is it possible to resolve the issue? |
Sorry, I thought you were saying that multiple docs were out of sync, you actually were saying that the docs were out of sync with the code. |
Yeah I think that's the initial discussion, but it also breaks the code. I guess it's related to: #739 |
I have fixed the examples and added a message in the changelog about the compatibility break. |
But does this have to be a compatibility break? Can render_kw be optional? It breaks many dependencies, other libraries (like wtf-peewee) and custom widgets. For example below is an example how it breaks wtf-peewee. I believe this can be an easy fix, don't use value unpacking, instead change it to something like:
Or I am sure there's a more elegant solution. I just feel like it creates a lot of unnecessary friction and requires many people to spend extra man-hours updating their code. (I'm sure it's about tens of thousands of man hours spent fixing bugs related to this breaking change creeping up, I know I just spent about an hour nailing it down and reporting to wtf-peewee and here) It also means that some unmaintained older projects that are otherwise working and mature can just stop functioning.
|
About your point on old unmaintained projects breaking, and people having to spend time to update their code, well there is a tradeoff to be made: I would not be against making render_kw optional for now, with a deprecation warning for a definitive removal in the next release. This way we can share the burden. Are you interested in opening a PR? |
First of all, thank you for developing such an outstanding library! I agree with your stance that breaking changes are sometimes essential. However, I'm concerned about the introduction of this breaking change within a minor release (despite I'm not sure if this project follows semantic versioning or not). In this case, by cutting a major release we would have likely avoided downstream dependencies issue. Thank you for your time and have a good day! 🙌 |
This fixes one of the out-of-date docstrings mentioned in wtforms#811 but not fixed in wtforms#812.
This fixes one of the out-of-date docstrings mentioned in wtforms#811 but not fixed in wtforms#812.
Barely a bug at all, but the docstring describes iter_choices here is out of sync with the code just below, which unpacks a tuple of length 4!
Also on the core class here!
Thanks for the great library!
The text was updated successfully, but these errors were encountered: