Skip to content

Remove frozen_string_literal comment from .arb templates #8600

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 26, 2025
Merged

Remove frozen_string_literal comment from .arb templates #8600

merged 1 commit into from
Jan 26, 2025

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Jan 4, 2025 •
edited
Loading

The suggestion to add the # frozen_string_literal: true comment at
the top of .arb files was a false positive for the following reasons:

  • It produced a warning.
  • It did not have any effect because .arb files are evaluated in a
    different context.

To make this work correctly, the comment should be added at the
framework level instead.

This change eliminates the warning when running specs with
RUBYOPT="-w".

Closes #8597

References:

@codecov Codecov
Copy link

codecov bot commented Jan 4, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (643fff6) to head (e7acb5f).
Report is 45 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #8600   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files         141      141           
  Lines        4076     4076           
=======================================
  Hits         4040     4040           
  Misses         36       36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tagliala tagliala force-pushed the chore/8597-fix-warnings branch from 548e33d to aeaa3e0 Compare January 4, 2025 20:49
@javierjulio javierjulio self-requested a review January 4, 2025 20:50
javierjulio
javierjulio approved these changes Jan 4, 2025
View reviewed changes
Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! What's the reason that the .arb templates don't need the # frozen_string_literal: true comment?

@tagliala
Copy link
Contributor Author

tagliala commented Jan 4, 2025 •
edited
Loading

What's the reason that the .arb templates don't need the # frozen_string_literal: true comment?

Ah, it does not have effect (tested on arbre), so the warning is legitimate

It has been highlighted here but I did not explicitly mentioned the reason in this commit message (I will reword): activeadmin/arbre@2bd6d87

I guess that .arb files are treated as string input and parsed somewhere else (like .erb) and so magic comments are ignored?

.erb templates are not affected because they are not treated as ruby files by rubocop: rubocop/rubocop@c7a0f00

I'm not sure if this is a problem with rubocop itself, or it is left to the user to always exclude .arb from that cop

https://github.com/rubocop/rubocop/blob/ccacb30ba92f0b2ec081923875ba8565f56c90e0/config/default.yml#L4039

Edit:

I guess that .arb files are treated as string input and parsed somewhere else (like .erb) and so magic comments are ignored?

yes, it looks so

https://github.com/activeadmin/arbre/blob/97da46273c0897aa5d695be56a055c74fb7e6085/lib/arbre/rails/template_handler.rb

Maybe it is worth to mention @deivid-rodriguez , which is also the author of rubocop/rubocop#5591

@tagliala tagliala force-pushed the chore/8597-fix-warnings branch from aeaa3e0 to eb4b117 Compare January 5, 2025 08:26
@tagliala
Copy link
Contributor Author

I'm going to split this so we can merge and fix

Lint/AmbiguousRegexpLiteral
Lint/UselessAssignment

and leave the decision open for .arb templates

@tagliala tagliala marked this pull request as draft January 12, 2025 11:39
@tagliala tagliala changed the title Enable some Lint cops to prevent warnings [WIP] .arb templates, RuboCop, and frozen string literals Jan 12, 2025
@tagliala tagliala force-pushed the chore/8597-fix-warnings branch from eb4b117 to 7ca9d82 Compare January 12, 2025 15:32
@tagliala
Copy link
Contributor Author

rubocop/rubocop#13684

@tagliala tagliala mentioned this pull request Jan 13, 2025
Closed
@tagliala tagliala force-pushed the chore/8597-fix-warnings branch 2 times, most recently from 82348f8 to fe1e22d Compare January 20, 2025 20:41
@tagliala tagliala changed the title [WIP] .arb templates, RuboCop, and frozen string literals [WIP] .arb templates, RuboCop, and frozen string literals - Pending new RuboCop release Jan 20, 2025
@tagliala tagliala changed the title [WIP] .arb templates, RuboCop, and frozen string literals - Pending new RuboCop release [WIP] Remove frozen_string_literal comment from .arb - Pending new RuboCop release Jan 20, 2025
@tagliala tagliala force-pushed the chore/8597-fix-warnings branch 2 times, most recently from 9a726ef to 0c57b1b Compare January 23, 2025 08:48
@tagliala tagliala changed the title [WIP] Remove frozen_string_literal comment from .arb - Pending new RuboCop release Remove frozen_string_literal comment from .arb Jan 24, 2025
@tagliala tagliala marked this pull request as ready for review January 24, 2025 21:47
@tagliala
Remove frozen_string_literal comment from .arb
e7acb5f 
The suggestion to add the `# frozen_string_literal: true` comment at
the top of `.arb` files was a false positive for the following reasons:

- It produced a warning.
- It did not have any effect because `.arb` files are evaluated in a
  different context.

To make this work correctly, the comment should be added at the
framework level instead.

This change eliminates the warning when running specs with
`RUBYOPT="-w"`.

Closes #8597

References:
- rails/rails#43725
- rubocop/rubocop#13699
@tagliala tagliala force-pushed the chore/8597-fix-warnings branch from 0c57b1b to e7acb5f Compare January 26, 2025 20:18
@tagliala tagliala merged commit 841a686 into master Jan 26, 2025
26 checks passed
@tagliala tagliala deleted the chore/8597-fix-warnings branch January 26, 2025 20:22
@tagliala tagliala changed the title Remove frozen_string_literal comment from .arb Remove frozen_string_literal comment from .arb templates Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierjulio javierjulio javierjulio approved these changes

Assignees
No one assigned
Labels
type template update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable some cops to prevent warnings
2 participants
@tagliala @javierjulio