Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upDetect enum fields as Selects rather than as Strings #1655
Conversation
|
This fails due to the vulnerabilities found in 6.0.2.2 version of some action gems, not sure if it's ok to bump them in this PR. |
|
Thanks for putting this up, just ran into this problem and it was really nice to see that someone had taken the time to make it better. |
|
Don't worry about the vulns. We just upgraded some gems, so a rebase might be enough. |
|
Regarding the PR, I think it could do the trick, although here's another potential idea. Since #1589 (not yet released in the gem) it's possible to pass a There's one problem I see: at the moment, Then we can have the field be something like this: state: Field::Select.with_options(
collection: ->(field) { field.resource.class.send(field.attribute.to_s.pluralize).keys) },
)OK, I'm not 100% sure of what I'm saying... The good thing about your approach is that it impacts only the generator, so no interfaces have to change. I have to think about this. Thoughts anyone? /cc @nickcharlton |
|
@pablobm alright, I think I figured it out. The only problem here is that it's pretty tricky to fetch Proc's definition as a String. The easiest way is to read the definition line from a file and parse it, but this forces us to define Procs in one line (or write a really good parser for multi-liners). The other solution would be to add a external gem to handle this, but it's an additional dependency. Please let me know what you think about this. (I have to violate the line length rule in |
|
A few style comments; I don't find myself with many implementation thoughts, so I'll defer to @pablobm here as he's already thinking about it. |
| remove_constants :Shipment, :ShipmentDashboard | ||
| end | ||
|
|
||
| it "properly handles collection procs option in the 'Select' field" do |
nickcharlton
May 22, 2020
Member
We can drop "properly" on this descriptor.
We can drop "properly" on this descriptor.
| enum: { searchable: false }, | ||
| # procs must be defined in one line! | ||
| enum: { searchable: false, | ||
| collection: ->(field) { field.resource.class.send(field.attribute.to_s.pluralize).keys } }, |
nickcharlton
May 22, 2020
Member
It's a little thing, but I wonder if we can use the lamdba do; end syntax here to achieve the same goal?
It's a little thing, but I wonder if we can use the lamdba do; end syntax here to achieve the same goal?
tadeusz-niemiec
May 22, 2020
•
Author
Contributor
It all depends on how good the parser will be, because I fetch raw file line there and parse it using regexp. We'll see how this idea works out and I'll try to implement parser for all possible syntaxes if it's the way to go.
It all depends on how good the parser will be, because I fetch raw file line there and parse it using regexp. We'll see how this idea works out and I'll try to implement parser for all possible syntaxes if it's the way to go.
| @@ -1,13 +1,12 @@ | |||
| require "rails/generators/named_base" | |||
|
|
|||
nickcharlton
May 22, 2020
Member
Can you add the the deleted line back in?
Can you add the the deleted line back in?
nickcharlton
changed the title
| enum: { searchable: false }, | ||
| # procs must be defined in one line! | ||
| enum: { searchable: false, | ||
| collection: ->(field) { field.resource.class.send(field.attribute.to_s.pluralize).keys } }, |
hound
bot
May 22, 2020
Metrics/LineLength: Line is too long. [108/80]
Metrics/LineLength: Line is too long. [108/80]
|
OK, I think I prefer your approach to what I mentioned. Your change only affects generators and doesn't change the internal APIs. It'll be safer particularly as we are starting to think about changing those APIs and we'll want to be careful about it. If you can address that last couple of comments, I think we can merge this. |
Do you mean that I should revert this to the previous version? Because actually it does what you mentioned, but imho it's pretty safe - the only difference in the select field is one ternary operator to check if proc requires any arguments. I've upgraded my regexp so it will handle btw some vulns were found |
|
Kaminari's update was merged in #1659 so if you rebase against current |
|
Oh, I see Hm, I have to admit I'm easy. Your change (sending the field as an argument) actually looks good, and I think it's the reasonable thing to do. I don't think we'd have done differently in the future. |
Fixes #1629
Improve enum field definition in the dashboard generator