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

Engine generator fix #1619

Merged
merged 1 commit into from
May 12, 2012
Merged

Conversation

jipiboily
Copy link

I fixed then engine generation of a view that had an hardcoded "title" attribute in it.

Was generated before (line 8):

<% if translation.title.present? %>

Now:

<% if translation.[my_attribute].present? %>

ie:

<% if translation.job_title.present? %>

I also removed some code duplication to keep that DRY.

@travisbot
Copy link

This pull request passes (merged 8a46207 into 332b512).

@ugisozols
Copy link
Member

This change will generate invalid code in case there's no string attribute - it'll just generate code like engine_name.

Current implementation will also generate absurd code if there's no string attribute like engine_name.title which will fail with undefined method 'title' for ... so this needs to fixed in either case.

@jipiboily
Copy link
Author

True.

Is there something to do about that? We're not to display text fields or date fields in the index, right? ;)

Open to suggestions.

@ugisozols
Copy link
Member

I was thinking about grabbing first attribute from the list in case there are no string ones. WDYT?

@jipiboily
Copy link
Author

Great, I'll fix that soon

@ugisozols
Copy link
Member

Thank you very much ❤️

@ugisozols
Copy link
Member

Summoning @parndt

@jipiboily
Copy link
Author

There seems to be a lot of things that break if there is no string...

  1. the :title_attribute is not generated
  2. if the first attribute (attributes.first.name) is not translated, it just doesn't show anything on index. (http://cl.ly/1y0S0V0f1S2f0I1W1c0t)
  3. probably more, it was just within a few minutes! ;)

@jipiboily
Copy link
Author

if we still want to use that, here is new first line of that file:

<% title_attribute = (title = attributes.detect { |a| a.type.to_s == "string" }).present? ? title.name : attributes.first.name %>

@ugisozols
Copy link
Member

I'll try to dig into this tomorrow as I'm starting to feel a bit sleepy.

@parndt
Copy link
Member

parndt commented Apr 25, 2012

It's supposed to use .title when there are no strings. It then creates def title in the model which prints a string telling you to override it.

@jipiboily
Copy link
Author

@parndt interesting...it crashed for me today when creating a new engine/extension...I'll try to take a look at that when I got some time.

@parndt
Copy link
Member

parndt commented May 10, 2012

@jipiboily thanks

@parndt
Copy link
Member

parndt commented May 12, 2012

This pull request seems great.

@parndt parndt merged commit 8a46207 into refinery:2-0-stable May 12, 2012
@ugisozols
Copy link
Member

@parndt iirc this pr wasn't ready to be merged. Main reason being this - "This change will generate invalid code in case there's no string attribute - it'll just generate code like engine_name."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants