Skip to content
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

Symbols support is incomplete #1127

Closed
romanbsd opened this issue Feb 9, 2015 · 9 comments · May be fixed by #1160
Closed

Symbols support is incomplete #1127

romanbsd opened this issue Feb 9, 2015 · 9 comments · May be fixed by #1160
Labels

Comments

@romanbsd
Copy link

romanbsd commented Feb 9, 2015

While using with the role_model gem, the following inconsistency has surfaced:

  • The user.roles returns a Set of Symbols
  • The User.valid_roles is an array of Symbols

While the selected_values (make_selected_values) is an array of Symbols, the Collections#collection converts the User.valid_roles to array of strings because

[Array, Fixnum, String].include?(raw_collection.first.class) #-> false

As a workaround, I provide the collection in the form:

User.valid_roles.map { |role| role.to_s.humanize }.zip(User.valid_roles)
@justinfrench
Copy link
Member

Sounds like we can treat symbols like strings. Would love to see a patch!

@romanbsd
Copy link
Author

I'm not familiar with the formtastic internals, thus I don't have a slightest idea what can break if the Symbol is added to the list above.

@Morred
Copy link
Contributor

Morred commented Jul 13, 2015

I gave this a try, so now arrays of symbols won't be mapped anymore but will be returned in their original form just like an array of strings would be. If that's not what you were aiming for here, I'm open for suggestions

@justinfrench
Copy link
Member

@Morred @romanbsd could one of you confirm that the #1160 patch fixes this issue in a real-world application?

@Morred
Copy link
Contributor

Morred commented Jul 13, 2015

In combination with the gem @romanbsd was mentioning?

@justinfrench
Copy link
Member

@Morred yeah, the fix looks good, but I'd love to know for sure this fixes the problem it attempts to solve before merging.

@Morred
Copy link
Contributor

Morred commented Jul 13, 2015

@justinfrench Absolutely, that makes sense. I'd love for @romanbsd to try it out because he could definitely tell whether it solves the problem in his current app or not. If he doesn't have time, I'll set up a small test app and check it myself.

@justinfrench justinfrench added this to the 4.0 beta milestone Mar 26, 2016
@justinfrench
Copy link
Member

@romanbsd? @Morred?

@Morred
Copy link
Contributor

Morred commented Mar 27, 2016

Ah damn, completely forgot about this one! If @romanbsd isn't here anymore, I guess I'll have to be the one to try it out

@justinfrench justinfrench removed this from the 4.0 beta milestone Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants