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

Clarify the sintax for magic comments in ERB files #435

Closed
afdev82 opened this issue Mar 28, 2022 · 16 comments
Closed

Clarify the sintax for magic comments in ERB files #435

afdev82 opened this issue Mar 28, 2022 · 16 comments

Comments

@afdev82
Copy link

afdev82 commented Mar 28, 2022

Hi all,

I've just upgraded to v1 (1.0.5) and I noticed that the health task now is failing due to some unused translations.
E.g., this piece of code was fine with the v0.9.37:

<%#-
i18n-tasks-use t('configurations.product_data_sheet_drawings')
i18n-tasks-use t('configurations.shop_drawings')
i18n-tasks-use t('configurations.client_drawings')
i18n-tasks-use t('configurations.production_drawings')
i18n-tasks-use t('configurations.spare_part_glass_drawings')
i18n-tasks-use t('configurations.price_overview_drawings')
i18n-tasks-use t('configurations.wall_template_drawings')
i18n-tasks-use t('configurations.dp_primer_drawings')
i18n-tasks-use t('configurations.dp_primer_black_drawings')
i18n-tasks-use t('configurations.dp_markers_drawings')
i18n-tasks-use t('configurations.dp_sandblasting_drawings')
i18n-tasks-use t('configurations.dp_pattern_drawings') -%>
<%= t "configurations.#{type}_drawings" %>

Now if I run bundle exec i18n-tasks health, I'm getting unused keys warnings:

Unused keys (20) | i18n-tasks v1.0.5
+--------+------------------------------------------+--------------------------------------------------------------+
| Locale | Key                                      | Value                                                        |
+--------+------------------------------------------+--------------------------------------------------------------+
|   de   | configurations.client_drawings           | Fertigungszeichnungen (Client Drawings)                      |
|   de   | configurations.dp_markers_drawings       | Digitaldrucker-Markierungen-Zeichnungen                      |
|   de   | configurations.dp_primer_black_drawings  | Digitaldrucker-Schwarz-Grundierung-Zeichnungen               |
|   de   | configurations.dp_primer_drawings        | Digitaldrucker-Grundierung-Zeichnungen                       |
|   de   | configurations.dp_sandblasting_drawings  | Digitaldrucker-Sandblasting-Zeichnungen                      |
|   de   | configurations.price_overview_drawings   | Preisübersicht                                               |
|   de   | configurations.production_drawings       | Produktionszeichnungen (Production Drawings)                 |
|   de   | configurations.shop_drawings             | Fertigungszeichnungen (Shop Drawings)                        |
|   de   | configurations.spare_part_glass_drawings | Zeichnungen der Glas Ersatzteile (Spare Part Glass Drawings) |
|   de   | configurations.wall_template_drawings    | Wandvorlage-Zeichnungen                                      |
|   en   | configurations.client_drawings           | Client Drawings                                              |
|   en   | configurations.dp_markers_drawings       | Digital Printer Markers Drawings                             |
|   en   | configurations.dp_primer_black_drawings  | Digital Printer Primer Black Drawings                        |
|   en   | configurations.dp_primer_drawings        | Digital Printer Primer Drawings                              |
|   en   | configurations.dp_sandblasting_drawings  | Digital Printer Sandblasting Drawings                        |
|   en   | configurations.price_overview_drawings   | Price Overview                                               |
|   en   | configurations.production_drawings       | Production Drawings                                          |
|   en   | configurations.shop_drawings             | Shop Drawings                                                |
|   en   | configurations.spare_part_glass_drawings | Spare Part Glass Drawings                                    |
|   en   | configurations.wall_template_drawings    | Wall template Drawings                                       |

I noticed that if I write the code in different ways I'm getting different results:

<%# i18n-tasks-use t('configurations.product_data_sheet_drawings')
i18n-tasks-use t('configurations.shop_drawings')
i18n-tasks-use t('configurations.client_drawings')
i18n-tasks-use t('configurations.production_drawings')
i18n-tasks-use t('configurations.spare_part_glass_drawings')
i18n-tasks-use t('configurations.price_overview_drawings')
i18n-tasks-use t('configurations.wall_template_drawings')
i18n-tasks-use t('configurations.dp_primer_drawings')
i18n-tasks-use t('configurations.dp_primer_black_drawings')
i18n-tasks-use t('configurations.dp_markers_drawings')
i18n-tasks-use t('configurations.dp_sandblasting_drawings')
i18n-tasks-use t('configurations.dp_pattern_drawings') %>
<%= t "configurations.#{type}_drawings" %>
$ bundle exec i18n-tasks health
Unused keys (2) | i18n-tasks v1.0.5
+--------+------------------------------------+-----------------------------------+
| Locale | Key                                | Value                             |
+--------+------------------------------------+-----------------------------------+
|   de   | configurations.dp_pattern_drawings | Digitaldrucker-Muster-Zeichnungen |
|   en   | configurations.dp_pattern_drawings | Digital Printer Pattern Drawings  |
+--------+------------------------------------+-----------------------------------+

In this way I'm still getting a warning for the last entry, but if I write the comments inside normal <% %> tags, it's fine:

<% # i18n-tasks-use t('configurations.product_data_sheet_drawings')
# i18n-tasks-use t('configurations.shop_drawings')
# i18n-tasks-use t('configurations.client_drawings')
# i18n-tasks-use t('configurations.production_drawings')
# i18n-tasks-use t('configurations.spare_part_glass_drawings')
# i18n-tasks-use t('configurations.price_overview_drawings')
# i18n-tasks-use t('configurations.wall_template_drawings')
# i18n-tasks-use t('configurations.dp_primer_drawings')
# i18n-tasks-use t('configurations.dp_primer_black_drawings')
# i18n-tasks-use t('configurations.dp_markers_drawings')
# i18n-tasks-use t('configurations.dp_sandblasting_drawings')
# i18n-tasks-use t('configurations.dp_pattern_drawings') %>
<%= t "configurations.#{type}_drawings" %>
$ bundle exec i18n-tasks health
✓ Good job! Every translation is in use.

Well, so what is the right way to use the magic comments inside the ERB files?
Thank you for your support!

@afdev82
Copy link
Author

afdev82 commented Mar 28, 2022

If I use the last syntax, then the code gets parsed by erb-lint and I get errors about wrong indentation, so I'm in a loop 🙈

@afdev82
Copy link
Author

afdev82 commented Mar 28, 2022

I've just found that if I write in this way

<%# i18n-tasks-use t('configurations.product_data_sheet_drawings')
# i18n-tasks-use t('configurations.shop_drawings')
# i18n-tasks-use t('configurations.client_drawings')
# i18n-tasks-use t('configurations.production_drawings')
# i18n-tasks-use t('configurations.spare_part_glass_drawings')
# i18n-tasks-use t('configurations.price_overview_drawings')
# i18n-tasks-use t('configurations.wall_template_drawings')
# i18n-tasks-use t('configurations.dp_primer_drawings')
# i18n-tasks-use t('configurations.dp_primer_black_drawings')
# i18n-tasks-use t('configurations.dp_markers_drawings')
# i18n-tasks-use t('configurations.dp_sandblasting_drawings')
# i18n-tasks-use t('configurations.dp_pattern_drawings') -%>
<%= t "configurations.#{type}_drawings" %>

changing the closing tag to -%> and use # for all magic comments, it's working.
But it seems more an hack than a solution.

@glebm
Copy link
Owner

glebm commented Mar 28, 2022

@davidwessman Seems like we might need to do a bit more AST hacking, can you please have a look at this and the other issue?

@davidwessman
Copy link
Collaborator

@davidwessman Seems like we might need to do a bit more AST hacking, can you please have a look at this and the other issue?

Yes, we need to decide on a format!

@glebm
Copy link
Owner

glebm commented Mar 28, 2022

Ideally we'd support all of the above

@davidwessman
Copy link
Collaborator

I have started working on handling the ERB-comments differently that should help for both this issue and #434 but did not manage to get it completely working yet.

@davidwessman
Copy link
Collaborator

@glebm Should all of these formats be supported?
I am thinking that I will make the erb-ast-parser handle as many of these cases as possible, maybe the first cases with ruby-comments inside one erb-tag can be handled by the normal comments-matching?

<!-- Inside the same ERB-tag -->
<%
  # i18n-tasks-use t('ruby_comment_same_erb_tag')
  inside_erb = FakeTranslate.single_inside
%>

<%
  # i18n-tasks-use t('multiple_ruby_comment_same_erb_tag1')
  # i18n-tasks-use t('multiple_ruby_comment_same_erb_tag2')
  # i18n-tasks-use t('multiple_ruby_comment_same_erb_tag3')
  inside_erb = FakeTranslate.multiple_inside
%>


<!-- ERB-comments before ERB-tag -->
<%# i18n-tasks-use t('erb_comment_before_erb_tag') %>
<%= FakeTranslate.with_erb_comment_before %>

<%# i18n-tasks-use t('multiple_erb_comment_before_erb_tag1') %>
<%# i18n-tasks-use t('multiple_erb_comment_before_erb_tag2') %>
<%# i18n-tasks-use t('multiple_erb_comment_before_erb_tag3') %>
<%= FakeTranslate.multiple_with_erb_comment_before %>

<%# i18n-tasks-use t('multiple_erb_comment_before_erb_tag1')
i18n-tasks-use t('multiple_erb_comment_before_erb_tag2')
i18n-tasks-use t('multiple_erb_comment_before_erb_tag3') %>
<%= FakeTranslate.multiple_one_tag_with_erb_comment_before %>


<!-- Ruby-comments before ERB-tag -->
<% # i18n-tasks-use t('ruby_comment_before_erb_tag') %>
<%= FakeTranslate.with_ruby_comment_before %>

<% # i18n-tasks-use t('multiple_ruby_comment_before_erb_tag1') %>
<% # i18n-tasks-use t('multiple_ruby_comment_before_erb_tag2') %>
<% # i18n-tasks-use t('multiple_ruby_comment_before_erb_tag3') %>
<%= FakeTranslate.multiple_with_ruby_comment_before %>

<%
  # i18n-tasks-use t('multiple_ruby_comment_before_erb_tag1')
  # i18n-tasks-use t('multiple_ruby_comment_before_erb_tag2')
  # i18n-tasks-use t('multiple_ruby_comment_before_erb_tag3')
%>
<%= FakeTranslate.multiple_one_tag_with_ruby_comment_before %>

@glebm
Copy link
Owner

glebm commented Apr 1, 2022

Yeah, I think all of the above are variations of how people write ERB comments, so they should ideally all be supported

@davidwessman
Copy link
Collaborator

Yeah, I think all of the above are variations of how people write ERB comments, so they should ideally all be supported

I got very confused when trying to fix both #434 and this one at the same time, but will try to tackle this one when #437 is merged.

davidwessman added a commit to davidwessman/i18n-tasks that referenced this issue Apr 2, 2022
@davidwessman
Copy link
Collaborator

@glebm I think #437 solves this issue as well, I added some more tests in #440 and it seems to work okay.

@davidwessman
Copy link
Collaborator

@afdev82 could you try to install from the main-branch and see if it works better for you now?

glebm pushed a commit that referenced this issue Apr 2, 2022
@afdev82
Copy link
Author

afdev82 commented Apr 4, 2022

All examples in my first post are working now, thank you!
The "hack" doesn't work anymore and in general if I write something like that:

<%# i18n-tasks-use t('multiple_erb_comment_before_erb_tag1')
# i18n-tasks-use t('multiple_erb_comment_before_erb_tag2')
# i18n-tasks-use t('multiple_erb_comment_before_erb_tag3') %>
<%= FakeTranslate.multiple_one_tag_with_erb_comment_before %>

Or

<%#
# i18n-tasks-use t('multiple_erb_comment_before_erb_tag1')
# i18n-tasks-use t('multiple_erb_comment_before_erb_tag2')
# i18n-tasks-use t('multiple_erb_comment_before_erb_tag3') %>
<%= FakeTranslate.multiple_one_tag_with_erb_comment_before %>

I don't know if they should be supported too.
Apart from that, it's way better now. Thanks!

@davidwessman
Copy link
Collaborator

@glebm Can we release a new version with these fixes?

@afdev82 You cannot mix ERB-comments (<%# %> ) with ruby-comments (# i18n-tasks-use t('what')).

So your examples would only work if you add a space before <%# -> <% # or remove the surplus # on each row inside <%# %>

@afdev82
Copy link
Author

afdev82 commented Apr 4, 2022

OK, I didn't know that.
Then it's all fine!

@davidwessman
Copy link
Collaborator

OK, I didn't know that.
Then it's all fine!

Oh, no worries! I had to find out the difference for this issue.

@glebm
Copy link
Owner

glebm commented Apr 4, 2022

Released v1.0.6!

@glebm glebm closed this as completed Apr 4, 2022
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

No branches or pull requests

3 participants