clarify the signature of the choice_value callable #6297
Conversation
backbone87
commented
Feb 22, 2016
| Q | A |
|---|---|
| Doc fix? | yes |
| New docs? | no |
| Applies to | 2.7+ |
| Fixed tickets |
| @@ -12,7 +12,10 @@ about this, but it might be handy when processing an API request (since you can | |||
| configure the value that will be sent in the API request). | |||
|
|
|||
| This can be a callable or a property path. See `choice_label`_ for similar usage. | |||
There was a problem hiding this comment.
Maybe it's better to remove "See choice_label_ for similar usage. completely. This would avoid talking about "differences" with the other options.
|
👍 to be more explicit with this option. |
|
Alright then ;) But at least, to quickly avoid errors, I hardly recommend to remove that line as I said in inline comment. |
| *If you use a callable for this option, it gets passed the choice as its **only** | ||
| argument*. This is a difference compared to the callables of `choice_label`_ and | ||
| `choice_name`_, which receive additional arguments. If ``null`` is used, an | ||
| incrementing integer is used as the name. |
There was a problem hiding this comment.
I agree that it would be best to have the section overhauled in general, but I also agree that this is a nice short-term fix. However, I suggest to change this a bit to something like this:
.. note::
In contrast to the ``choice_label`` and ``choice_name`` option when using
a callable for the ``choice_value`` option, the actual choice is the **only**
argument passed to it.What do you think?
There was a problem hiding this comment.
👍
This is a detail but I would say :
.. note::
In contrast to other choice options (e.g ``choice_label``), the callable for
the ``choice_value`` option takes as **only** argument the actual choice.Because the actual enumeration misses choice_attr (and maybe future other(s) ;).
Anyway please remove See choice_label_ for similar usage., as I really think it can confuse new comers.
|
i have applied your feedback, can be autosquashed for clean history |
|
|
||
| In contrast to other choice options (e.g. `choice_label`_), the callable for | ||
| the ``choice_value`` option takes as **only** argument the actual choice, which | ||
| is also the value that gets passed to your model after submitting the form. |
There was a problem hiding this comment.
I would replace after by when.
Beside that 👍
|
done |
|
|
||
| In contrast to other choice options (e.g. `choice_label`_), the callable for | ||
| the ``choice_value`` option takes as **only** argument the actual choice, which | ||
| is also the value that gets passed to your model when submitting the form. |
There was a problem hiding this comment.
Sorry, but now I'm re-reading it and I think that ", which is also the value that gets passed to your model when submitting the form" is too confusing. It better should be defined outside of this note, when clarifying the responsibility of this option to return a unique string representation of the choice.
There was a problem hiding this comment.
@backbone87 Can you reword this a bit?
…one87) This PR was squashed before being merged into the 2.7 branch (closes #6297). Discussion ---------- clarify the signature of the choice_value callable | Q | A | | --- | --- | | Doc fix? | yes | | New docs? | no | | Applies to | 2.7+ | | Fixed tickets | | Commits ------- 5cf83f6 clarify the signature of the choice_value callable
|
Thanks! I've just merged this, and tried to clarify further at sha: 6365c01 |
This PR was merged into the 3.4 branch. Discussion ---------- [Form] Fixed some phpdocs | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | symfony/symfony-docs#6393 ref symfony/symfony-docs#6144, symfony/symfony-docs#6297, #14050 Commits ------- b9162e8 [Form] Fixed some phpdocs