Incorrect docstring?

[Nicholas-Read]

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!

[davidism]

You can submit a PR to make this consistent.

[yacine-harbi]

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():

[aminalaee]

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?

[davidism]

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.

[aminalaee]

Yeah I think that's the initial discussion, but it also breaks the code. I guess it's related to: #739

[azmeuk]

I have fixed the examples and added a message in the changelog about the compatibility break.

[Nixellion]

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:

for choice in field.iter_choices():
    val = choice[0]
    label = choice[1]
    selected = choice[2]
    if len(choice) > 3:
        render_kw = choice[4]
    else:
        render_kw = {}

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.

Traceback (most recent call last):
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask\app.py", line 2190, in wsgi_app
    response = self.full_dispatch_request()
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask\app.py", line 1486, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask\app.py", line 1484, in full_dispatch_request
    rv = self.dispatch_request()
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask\app.py", line 1469, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\base.py", line 69, in inner
    return self._run_view(f, *args, **kwargs)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\base.py", line 369, in _run_view
    return fn(self, *args, **kwargs)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\model\base.py", line 2124, in create_view
    return self.render(template,
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\base.py", line 308, in render
    return render_template(template, **kwargs)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask\templating.py", line 151, in render_template
    return _render(app, template, context)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask\templating.py", line 132, in _render
    rv = template.render(context)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\jinja2\environment.py", line 1301, in render
    self.environment.handle_exception()
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\jinja2\environment.py", line 936, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\model\create.html", line 3, in top-level template code
    {% from 'admin/lib.html' import extra with context %} {# backward compatible #}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\master.html", line 1, in top-level template code
    {% extends admin_base_template %}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\base.html", line 39, in top-level template code
    {% block page_body %}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\base.html", line 75, in block 'page_body'
    {% block body %}{% endblock %}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\model\create.html", line 22, in block 'body'
    {% block create_form %}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\model\create.html", line 23, in block 'create_form'
    {{ lib.render_form(form, return_url, extra(), form_opts) }}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\jinja2\runtime.py", line 777, in _invoke
    rv = self._func(*arguments)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\lib.html", line 240, in template
    {% call form_tag(action=action) %}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\jinja2\runtime.py", line 777, in _invoke
    rv = self._func(*arguments)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\lib.html", line 209, in template
    {{ caller() }}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\jinja2\runtime.py", line 777, in _invoke
    rv = self._func(*arguments)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\lib.html", line 241, in template
    {{ render_form_fields(form, form_opts=form_opts) }}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\jinja2\runtime.py", line 777, in _invoke
    rv = self._func(*arguments)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\lib.html", line 201, in template
    {{ render_field(form, f, kwargs) }}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\jinja2\runtime.py", line 777, in _invoke
    rv = self._func(*arguments)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\flask_admin\templates\bootstrap4\admin\lib.html", line 146, in template
    {{ field(**kwargs) | safe }}
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\wtfpeewee\fields.py", line 252, in __call__
    return self.widget(self, **kwargs)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\wtfpeewee\fields.py", line 153, in __call__
    return super(ChosenSelectWidget, self).__call__(field, **kwargs)
  File "d:\Projects\Web\project_name\project_name_refactor\venv\lib\site-packages\wtforms\widgets\core.py", line 365, in __call__
    for val, label, selected, render_kw in field.iter_choices():
ValueError: not enough values to unpack (expected 4, got 3)

[azmeuk]

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:
People ask for bugfixes and features in wtforms, that leads to refactoring, that sometimes leads to compatibility breakages. Keeping the old API puts the maintenance burden on the wtforms maintainers instead of the maintainers of projects using wtforms.

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?

[guidoiaquinti]

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: People ask for bugfixes and features in wtforms, that leads to refactoring, that sometimes leads to compatibility breakages. Keeping the old API puts the maintenance burden on the wtforms maintainers instead of the maintainers of projects using wtforms.

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! 🙌