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 user tags - use defaulted keys if appropriate #22003

Merged
merged 1 commit into from
Dec 17, 2021

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Dec 7, 2021

@mkouba
Copy link
Contributor Author

mkouba commented Dec 7, 2021

@FroMage This is the initial attempt to solve #21861. It should solve the problem with class and size. Furthermore, it's possible to configure that a user tag should not delegate to the parent context, e.g. {#test class="myclass" _isolated=true /} or {#test class="myclass" _isolated /}. I'm still not convinced it should be the default behavior but if we decide to do so it will be a one-liner. And I'm also not sure about the name of the config property - _isolated should be quite unambiguous one-word but it's not very intuitive.

@FroMage
Copy link
Member

FroMage commented Dec 13, 2021

OK thanks. This is missing docs though. And I really think tags should be isolated by default, as every programming language function.

Naturally, for tags that accept a body, the body should be resolved in the context of the parent, and not the tag. But again, exactly the same scoping rules as Java and most modern programming languages.

@mkouba
Copy link
Contributor Author

mkouba commented Dec 14, 2021

This is missing docs though.

Yes, it's not documented because I'm still not convinced of the param name... Maybe something like _skipParent or _noParent would be more understandable?

Naturally, for tags that accept a body, the body should be resolved in the context of the parent, and not the tag. But again, exactly the same scoping rules as Java and most modern programming languages.

Well, I'll be repeating myself but tags are IMO not functions but more like a specialized include directives. I get your point but I'm hesitating to change the default behavior and introduce another breaking change. Moreover, we would have to follow the same rules for {#include} sections...

@FroMage
Copy link
Member

FroMage commented Dec 14, 2021

I don't know what external scope is visible from includes, that's not obvious from https://quarkus.io/guides/qute-reference#include_helper

In general, I really think we should stick to programming language semantics, especially Java, because that's what make the most sense for Quarkus devs.

@mkouba
Copy link
Contributor Author

mkouba commented Dec 14, 2021

I don't know what external scope is visible from includes, that's not obvious from https://quarkus.io/guides/qute-reference#include_helper

It's simply the parent scope. The docs mention "The included template can reference data from the current context.".

In general, I really think we should stick to programming language semantics, especially Java, because that's what make the most sense for Quarkus devs.

+1 but it's a pity we did not catch this a few dozen versions back :-(

- i.e. for params that define no key and contain only single part
- also use Mapper instead of Map for the data passed to a template
- Mapper value resolver has priority 15, i.e. is called before a
generated value resolver
- resolves quarkusio#21855
- related to quarkusio#21861
@mkouba
Copy link
Contributor Author

mkouba commented Dec 15, 2021

I've updated the docs. Let's merge this now and we can decide to change the default behavior later... @FroMage WDYT?

@mkouba mkouba added this to the 2.7 - main milestone Dec 15, 2021
@FroMage
Copy link
Member

FroMage commented Dec 15, 2021

Well, now you've pretty much set it in stone in the docs that the current behaviour is dynamic scoping, so people may start using it. If you agree that this is the direction we should go do, we should make the change now that people are most likely not relying on dynamic scoping for user tags, and perhaps include a configuration setting to enable the old behaviour in case people depend on it?

@mkouba
Copy link
Contributor Author

mkouba commented Dec 15, 2021

Well, now you've pretty much set it in stone in the docs that the current behaviour is dynamic scoping, so people may start using it.

The current behaviour is "dynamic scoping" and it was like that since the beginning.

If you agree that this is the direction we should go do, we should make the change now that people are most likely not relying on dynamic scoping for user tags, and perhaps include a configuration setting to enable the old behaviour in case people depend on it?

This PR merely clarifies the current behavior and I am still not sure which direction to go. But I agree that we should decide in the scope of 2.7. I can create a new discussion topic and we can create a follow-up PR if we decide to do a breaking change.

@mkouba
Copy link
Contributor Author

mkouba commented Dec 16, 2021

I've created #22285

@mkouba mkouba requested a review from gastaldi December 16, 2021 16:36
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.

LGTM

@mkouba
Copy link
Contributor Author

mkouba commented Dec 17, 2021

I'm going to merge this one, let's continue the discussion in #22285.

@mkouba mkouba merged commit 0b90878 into quarkusio:main Dec 17, 2021
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.

Qute: shorter tag invocation
3 participants