feature multiple selection with serialized arrays #3

Open
wants to merge 1 commit into
from

Projects

None yet

3 participants

@brueckenschlaeger23

As discussed, for multiple selection and restricted by assignable_values, let's use a serialized array.

Regarding usage, please check the README.md #multiple-selection. For all cases listed you will find according tests added to the active_record_spec.rb.

For any question or further required changes, don't hesitate to contact me.

@henning-koch
Member

Hey Michael,

thanks for the commit. Code looks good except some minor things, but I'm deeply unhappy that e.g. object.humanized_fields is now ambiguous (list of humanized values that are assignable vs. humanization of chosen values). Since serialized array fields are almost always named in plural form, this will be source for confusion whenever someone uses this new feature.

I would rename the method to something else, but this would break existing API clients.

I need to think about this some more before accepting this, sorry.

@brueckenschlaeger23

Hi Henning,

thanks for the feedback. Please consider, that this problem used to exist before for all attributes that could not be properly pluralized like "news". The small difference was that those attributes basically broke assignable_values.

Similar to the rails approach to give a different name for index actions (suffix "index"), I added that the humanized_XXX methods will be prefixed with "available" in those cases. (I would be very happy with a different naming by default though; without ambiguous pluralize at all.)

Have a look at scalar_attribute.rb:74

def humanized_values_method_name

where the name is set and the test active_record_spec.rb:489

        context 'when the property cannot be pluralized' do

where I test the prefixing.

Thus, it's not API breaking but it actually fixes assignable_values for non-pluralizable attributes like "news", too.

@henning-koch
Member

I agree that the pluralized method name was a poor name choice to begin with.

I also saw that you worked around this in your code. What I don't think is a good idea to mix too many adjectives (assignable / available) and also the method name shouldn't depend on whether the field name is pluralizable or not. This is a source for confusion of errors.

As much as it hurts me to break the API I'm contemplating renaming all the methods like humanized_assignable_foos. Maybe keep the old version around with deprecation warnings.

@brueckenschlaeger23

Good choice! Me, too, I prefer a solution without a confusing / pluralizable-dependent method name and humanized_assignable_foos seems perfect to me.

If it helps to speed up the process, I can update our pull-request to include the above naming and to add deprecation warnings respectively. Or do you prefer that we wait for your change until we update our Pull Request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment