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

Update generators with new Gettext API #5902

Merged

Conversation

Comment on lines 43 to 44
layouts: [html: <%= @web_namespace %>.Layouts]
<%= if @gettext do %>
Copy link
Member

Choose a reason for hiding this comment

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

The new lines are tricky to get right, but I am almost sure this is what is needed. You have to replicate this below.

Suggested change
layouts: [html: <%= @web_namespace %>.Layouts]
<%= if @gettext do %>
layouts: [html: <%= @web_namespace %>.Layouts]<%= if @gettext do %>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With --gettext:

    quote do
      # Translation
      use Gettext, backend: GtestWeb.Gettext
      
      # HTML escaping functionality
      import Phoenix.HTML
      # Core UI components
      import GtestWeb.CoreComponents

      # Shortcut for generating JS commands
      alias Phoenix.LiveView.JS

      # Routes generation with the ~p sigil
      unquote(verified_routes())
    end

With --no-gettext:

    quote do
      # HTML escaping functionality
      import Phoenix.HTML
      # Core UI components
      import GtestWeb.CoreComponents

      # Shortcut for generating JS commands
      alias Phoenix.LiveView.JS

      # Routes generation with the ~p sigil
      unquote(verified_routes())
    end

I think we're good? I would love to keep the use above import as it seems like it's a pretty common ordering. 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(this is with changes I’m about to push to be clear!)

@@ -14,10 +14,10 @@ defmodule <%= @web_namespace %>.CoreComponents do

Icons are provided by [heroicons](https://heroicons.com). See `icon/1` for usage.
"""
use Phoenix.Component
use Phoenix.Component<%= if @gettext do %>
use Gettext, backend: <%= @web_namespace %>.Gettext<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

This is perfect!

@@ -14,10 +14,10 @@ defmodule <%= @web_namespace %>.CoreComponents do

Icons are provided by [heroicons](https://heroicons.com). See `icon/1` for usage.
"""
use Phoenix.Component
use Phoenix.Component<%= if @gettext do %>
use Gettext, backend: <%= @web_namespace %>.Gettext<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

This is perfect!

@whatyouhide
Copy link
Contributor Author

whatyouhide commented Aug 19, 2024

In the meantime, tested again with the latest Gettext changes and it looks great:

$ mix xref trace lib/gtest_web/components/core_components.ex
lib/gtest_web/components/core_components.ex:1: call GtestWeb.CoreComponents.__phoenix_component_verify__/1 (compile)
lib/gtest_web/components/core_components.ex:18: alias GtestWeb.Gettext (runtime)
lib/gtest_web/components/core_components.ex:568: alias GtestWeb.Gettext (runtime)
lib/gtest_web/components/core_components.ex:570: alias GtestWeb.Gettext (runtime)
mix xref trace lib/gtest_web/components/core_components.ex  1.70s user 1.32s system 127% cpu 2.363 total

$ mix xref trace lib/gtest_web/controllers/page_controller.ex
lib/gtest_web/controllers/page_controller.ex:1: call GtestWeb.PageController.__phoenix_verify_routes__/1 (compile)
lib/gtest_web/controllers/page_controller.ex:2: require GtestWeb (export)
lib/gtest_web/controllers/page_controller.ex:2: alias GtestWeb.Endpoint (runtime)
lib/gtest_web/controllers/page_controller.ex:2: alias GtestWeb.Gettext (runtime)
lib/gtest_web/controllers/page_controller.ex:2: alias GtestWeb.Layouts (runtime)
lib/gtest_web/controllers/page_controller.ex:2: alias GtestWeb.Router (runtime)
lib/gtest_web/controllers/page_controller.ex:2: call GtestWeb.__using__/1 (compile)
lib/gtest_web/controllers/page_controller.ex:2: call GtestWeb.static_paths/0 (compile)

@whatyouhide
Copy link
Contributor Author

@josevalim this is ready for another review. I posted some comments with results of the formatting and I think it looks good!

@josevalim
Copy link
Member

@whatyouhide some integration tests are failing because the generated file is not formatted. :) I will try to drop some comments but it seems to be trailing whitespace.

@whatyouhide
Copy link
Contributor Author

@josevalim let me run those locally and I'll figure it out

quote do<%= if @gettext do %>
# Translation
use Gettext, backend: <%= @web_namespace %>.Gettext
<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

This will leave trailing whitespace. I think the <%= end %> needs to be at the end of the previous line.

import <%= @web_namespace %>.Gettext<% end %>
<%= if @gettext do %>
use Gettext, backend: <%= @web_namespace %>.Gettext
<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

This will leave trailing whitespace before and after. I think the if and <%= end %> needs to be at the end of the previous line.

quote do<%= if @gettext do %>
# Translation
use Gettext, backend: <%= @web_namespace %>.Gettext
<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

This will leave trailing whitespace. I think the <%= end %> needs to be at the end of the previous line.


import Plug.Conn<%= if @gettext do %>
import <%= @web_namespace %>.Gettext<% end %>
<%= if @gettext do %>
Copy link
Member

Choose a reason for hiding this comment

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

This will leave trailing whitespace. I think the <%= end %> needs to be at the end of the previous line.

@josevalim josevalim merged commit bb099f5 into phoenixframework:main Aug 20, 2024
7 checks passed
@josevalim
Copy link
Member

This will be a massive improvement to Phoenix apps. Thank you @whatyouhide and @maennchen! 💚 💙 💜 💛 ❤️

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.

2 participants