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

Qute type-safe messages - change the default bundle name strategy #31299

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Feb 20, 2023

  • use an approach similar to type-safe templates
  • default bundle name of nested class includes the simple names declaring classes

For example, the default bundle name of Messages from the following class is msg_Views_Index and in a template {msg_Views_Index:hello(name)}.

import io.quarkus.qute.CheckedTemplate;
import io.quarkus.qute.TemplateInstance;
import io.quarkus.qute.i18n.Message;
import io.quarkus.qute.i18n.MessageBundle;

public class Views {

    public static class Index {

        @CheckedTemplate
        static class Templates {

            static native TemplateInstance index(String name);

        }

        @MessageBundle
        public interface Bundle {

            @Message("Hello {name}!")
            String hello(String name);
        }
    }
}

@quarkus-bot quarkus-bot bot added area/documentation area/qute The template engine labels Feb 20, 2023
@mkouba
Copy link
Contributor Author

mkouba commented Feb 20, 2023

FYI @FroMage. This PR does not solve your problem but it's a first step ;-).

@github-actions
Copy link

github-actions bot commented Feb 20, 2023

🙈 The PR is closed and the preview is expired.

@mkouba mkouba force-pushed the qute-message-bundle-defaulted-name branch from 4fa1221 to a253656 Compare February 20, 2023 14:55
@mkouba
Copy link
Contributor Author

mkouba commented Feb 20, 2023

Marked as a breaking change because previously the default name msg was always used.

@quarkus-bot

This comment has been minimized.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

lgtm, I left comment rather out of curiosity whether you know something I don't...

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Maybe it's me, but I find it difficult to use {msg_Views_Index:hello(name)} in a template.

Here is a suggestion: Would it make sense to use just {msg:hello(name)} in the template and the resolver perform the Message Bundle lookup based on the same strategy you're proposing here?

docs/src/main/asciidoc/qute-reference.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/qute-reference.adoc Show resolved Hide resolved
@mkouba
Copy link
Contributor Author

mkouba commented Feb 20, 2023

Maybe it's me, but I find it difficult to use {msg_Views_Index:hello(name)} in a template.

You're not the only one - Stef has the same complaints ;-).

Here is a suggestion: Would it make sense to use just {msg:hello(name)} in the template and the resolver perform the Message Bundle lookup based on the same strategy you're proposing here?

There are few problems with this proposal.

  1. A message bundle is not bound to a template; it's a global construct. You can even use multiple bundles in a template.
  2. Namespace resolvers are also global and do not know anything about the strategies used to extract the message bundle name.
  3. Messages are validated at build time, so if you use {msg:hello(name)} then we would need to translate the expression to {msg_Views_Index:hello(name)} during validation somehow...

That said, it's probably doable but (1) not so easy, (2) I'd prefer to introduce a more general solution (maybe something like "local namespace aliases"?). So this PR is not a final solution but a first step to make it easier to use multiple nested message bundles in your app.

- use an approach similar to type-safe templates
- the default bundle name of a nested class starts with "msg" and includes the simple names
of all declaring classes in the hierarchy
@mkouba mkouba force-pushed the qute-message-bundle-defaulted-name branch from a253656 to 709b87b Compare February 21, 2023 08:25
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 21, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@mkouba
Copy link
Contributor Author

mkouba commented Feb 21, 2023

@gastaldi Like I mentioned in the quarkiverse/quarkus-renarde#3 (comment) - I think that a workaround is to use the generated message bundle impl directly (i.e. do not use the namespace at all).

@mkouba mkouba merged commit 37343bb into quarkusio:main Feb 21, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 21, 2023
@FroMage
Copy link
Member

FroMage commented Feb 21, 2023

Errr, the given example is not idiomatic, at least in the case of Renarde. This would be:

public class Controller {
        @CheckedTemplate
        static class Templates {
            static native TemplateInstance index(String name);
        }
}

I'm not sure what the Index class you used as example does, besides grouping a single template and its messages.

The suggestion I made was rather to group messages in the same class name as the template:

public class Controller {
        @CheckedTemplate
        static class Templates {
            static native TemplateInstance index(String name);
            @MessageBundle
            static class index {
              @Message
              String greeting();
            } 
        }
}

@mkouba
Copy link
Contributor Author

mkouba commented Feb 21, 2023

The suggestion I made was rather to group messages in the same class name as the template:

public class Controller {
        @CheckedTemplate
        static class Templates {
            static native TemplateInstance index(String name);
            @MessageBundle
            static class index {
              @Message
              String greeting();
            } 
        }
}

But this would result in the bundle name msg_Controller_Templates (if we follow the logic used for checked templates) 🤔.

TBH I don't like any of these "nested" proposals. It's a mess... anyway, it's not set in stone yet! So we can definitely change/revert the behavior.

@FroMage
Copy link
Member

FroMage commented Feb 21, 2023

Well, ideally it'd result in Controller_index.

The alternative, to still group messages by view, is:

public class Controller {
        @CheckedTemplate
        static class Templates {
            static native TemplateInstance index(String name);
        }
            @MessageBundle
            static class index {
              @Message
              String greeting();
            } 
}

@mkouba
Copy link
Contributor Author

mkouba commented Mar 1, 2023

Well, ideally it'd result in Controller_index.

The alternative, to still group messages by view, is:

public class Controller {
        @CheckedTemplate
        static class Templates {
            static native TemplateInstance index(String name);
        }
            @MessageBundle
            static class index {
              @Message
              String greeting();
            } 
}

Ok, we could just take all the classes in the hierarchy and do not add the msg_ prefix VS the current behavior - skip the message bundle interface name and use the msg_ prefix.

I.e. you would then use Controller_index:greeting in the template.

@FroMage WDYT?

mkouba added a commit to mkouba/quarkus that referenced this pull request Mar 1, 2023
mkouba added a commit to mkouba/quarkus that referenced this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants