Skip to content

PrismScanner: Contextual parsing for Rails#565

Merged
davidwessman merged 3 commits intomainfrom
controller-contextual-parsing
Jun 10, 2024
Merged

PrismScanner: Contextual parsing for Rails#565
davidwessman merged 3 commits intomainfrom
controller-contextual-parsing

Conversation

@davidwessman
Copy link
Copy Markdown
Collaborator

@davidwessman davidwessman commented May 7, 2024 •
edited
Loading

  • Adding a scanner based on Prism.
  • It supports
    • before_action in controllers
    • translations in nested method calls
    • model_name.human
    • human_attribute_name

Note to reviewers

The scanner could replace all our calls to the parser gem today and could in the future also replace our matchers.
The harder part is to make it extendable, should we allow the end user to implement their own Prism::Visitor that would support some new use case?

I left the email subjects and ViewComponent for a smaller future PR, but I think this is a nice base to start testing it out on some real apps and evaluate if this is something we want to move towards.

TODO

  • Handle comments
  • Add magic comment to skip parsing file
  • Handle all arguments to translation method
  • Handle Rails-model calls
  • Handle more before_actions, or warn that they cannot be processed

Future features

  • Handle email subjects
  • Handle ViewComponent translations
  • Handle default argument

@davidwessman davidwessman force-pushed the controller-contextual-parsing branch from 36d0316 to fadc07d Compare May 8, 2024 05:28
@glebm
Copy link
Copy Markdown
Owner

glebm commented May 10, 2024

We already have a Ruby parser dependency, consider reusing it rather than adding another one.

@davidwessman
Copy link
Copy Markdown
Collaborator Author

davidwessman commented May 10, 2024

We already have a Ruby parser dependency, consider reusing it rather than adding another one.

Prism is a standard ruby gem and is included by default from Ruby 3.3 and can be installed from Ruby 3.0.
I think we should investigate moving away from parser towards it.

I will do this experiment in Prism and then I can rewrite it to use the old parser.

When parsing controllers I would like to return a key or a fallback, e.g. if there is a relative key in a method name I would like the key with either the method name included or with just the controller namespace.

class EventsController < ApplicationController
  def show
    t(".relative")
  end
end

Then I would like it to first check for events.show.relative, but if that does not exist, fallback to events.relative.
Do you have any idea where to start with adding such behaviour?

@davidwessman davidwessman force-pushed the controller-contextual-parsing branch 2 times, most recently from a325228 to 1fb6f4b Compare May 10, 2024 05:53
@glebm
Copy link
Copy Markdown
Owner

glebm commented May 10, 2024 •
edited
Loading

I don't mind dropping support for older rubies, but does Prism support JRuby?
If so, switching to Prism throughout might be a good idea

Then I would like it to first check for events.show.relative, but if that does not exist, fallback to events.relative.

Absolute keys are resolved by calling absolute_key:

i18n-tasks/lib/i18n/tasks/scanners/relative_keys.rb

Lines 7 to 12 in 2a9a9e0

# @param key [String] relative i18n key (starts with a .)
# @param path [String] path to the file containing the key
# @param roots [Array<String>] paths to relative roots
# @param calling_method [#call, Symbol, String, false, nil]
# @return [String] absolute version of the key
def absolute_key(key, path, roots: config[:relative_roots],

Sounds like the code calling absolute_key would need to have access to the data:

i18n-tasks/lib/i18n/tasks/data.rb

Lines 11 to 13 in 2a9a9e0

# I18n data provider
# @see I18n::Tasks::Data::FileSystem
def data

@glebm
Copy link
Copy Markdown
Owner

glebm commented May 10, 2024

Looks like Prism on JRuby is still a work-in-progress: http://blog.jruby.org/2024/02/jruby-prism-parser

@davidwessman davidwessman force-pushed the controller-contextual-parsing branch 2 times, most recently from 6946055 to 305b747 Compare May 11, 2024 10:03
@jaredmoody
Copy link
Copy Markdown
Contributor

jaredmoody commented May 17, 2024

+1 for Prism! The speed gains rubocop is getting from using it are 👀

@davidwessman
Copy link
Copy Markdown
Collaborator Author

davidwessman commented May 18, 2024

Looks like Prism on JRuby is still a work-in-progress: http://blog.jruby.org/2024/02/jruby-prism-parser

I think this means that JRuby are working on replacing their parser with Prism, for their compilation.
Prism can still handle all syntax that JRuby supports and can run in JRuby, so for our use-case it would be okay.

@davidwessman davidwessman force-pushed the controller-contextual-parsing branch 3 times, most recently from 5d7ae4e to 2018d84 Compare May 19, 2024 06:22
@davidwessman davidwessman force-pushed the controller-contextual-parsing branch 2 times, most recently from ac7c37e to 6bcc609 Compare June 6, 2024 20:09
@davidwessman davidwessman changed the title Contextual parsing of Rails controllers PrismScanner: Adds support for more Rails translation behaviour Jun 6, 2024
@davidwessman davidwessman changed the title PrismScanner: Adds support for more Rails translation behaviour PrismScanner: Contextual parsing for Rails Jun 6, 2024
@davidwessman davidwessman force-pushed the controller-contextual-parsing branch 3 times, most recently from 87424f4 to 1ba8b45 Compare June 9, 2024 13:42
@davidwessman
PrismScanner: Contextual parsing for Rails
e2dcf07 
Adds a scanner that supports:
  - `before_action` in controllers
  - translations in nested method calls
  - `model_name.human`
  - `human_attribute_name`
@davidwessman davidwessman force-pushed the controller-contextual-parsing branch from 1ba8b45 to e2dcf07 Compare June 9, 2024 14:18
davidwessman
davidwessman commented Jun 9, 2024
View reviewed changes
Comment thread lib/i18n/tasks/scanners/prism_scanners/visitor.rb Outdated
davidwessman
davidwessman commented Jun 9, 2024
View reviewed changes
Comment thread spec/prism_scanner_spec.rb
%w[
activerecord.attributes.event.title
activerecord.models.event.one
activerecord.models.event.other
Copy link
Copy Markdown
Collaborator Author

@davidwessman davidwessman Jun 9, 2024

Choose a reason for hiding this comment

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

Maybe we would like some different way to return a pluralized translation, e.g. that it would just return activerecord.models.event with some pluralized setting and then this can be included in the tree.

Copy link
Copy Markdown
Owner

@glebm glebm Jun 10, 2024

Choose a reason for hiding this comment

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

Yeah, I think that would be necessary to support pluralized keys properly.
Can be done in a follow-up PR

@davidwessman davidwessman marked this pull request as ready for review June 9, 2024 14:27
@davidwessman davidwessman requested a review from glebm June 9, 2024 14:29
@davidwessman
Copy link
Copy Markdown
Collaborator Author

davidwessman commented Jun 9, 2024

@glebm Now I feel like I am ready for a review of this as a beta-feature.

I am sorry for a large PR, please let me know if you have any suggestions for making it smaller and more reviewable.

@davidwessman davidwessman merged commit 6a46c61 into main Jun 10, 2024
@glebm glebm deleted the controller-contextual-parsing branch June 10, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glebm glebm glebm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@davidwessman @glebm @jaredmoody