-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
text-minimessage: Add Placeholder to format Numbers and TemporalAccessors #709
Conversation
...minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/Placeholder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I don't really think this fits in Placeholder
as nothing else in this class supports arguments. Although I suppose it technically it a placeholder as it just inserts one value but just optionally has formatting? Not sure, would like to hear others' thoughts.
...minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/Placeholder.java
Outdated
Show resolved
Hide resolved
...minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/Placeholder.java
Outdated
Show resolved
Hide resolved
maybe we can have a TagResolvers class for "utils" like this? |
I moved it to a new Formatter class. This might be not the final name, Maybe "FormatterTags" would be better? And I added a syntax to allow formatting the decimal and grouping separator. The syntax is currently: Should I convert the decimal and grouping separator to single arguments? So the syntax would be instead |
text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/Formatter.java
Outdated
Show resolved
Hide resolved
e4eb0ac
to
9d3966f
Compare
9d3966f
to
1be2060
Compare
Currently waiting for #735 |
Can you explain why you chose to make these pre-process tags? I think they'd be better suited as |
The idea is this: |
Hm, that makes sense. Do we want style to bleed through though? it might make more sense to just parse the output of the formatter as components. |
I'm not sure. Using PreProcess tags should be flexible here. E.g. a gradient tag should work with the formatting in the tag. We can change it to a inserting tag instead too, |
Gradient tags should work with uncoloured I'd like to avoid pre-process tags since they have such special-cased handling within the parser at the moment, which causes problems with error message offsets mostly. |
Yes, you're correct. I'll change it to |
ba868f7
to
1064dc3
Compare
I rebased the PR and squashed the commits. The class is now Formatter and I changed some stuff to be more intuitive. E.g. detecting automatically whether you provide a locale arg, a formatting arg or both. |
Nice work ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpicks before merging!
text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/Formatter.java
Outdated
Show resolved
Hide resolved
text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/Formatter.java
Outdated
Show resolved
Hide resolved
I agree, that would be nice too. I'll take a look at the weekend. Should be pretty easy |
Added ChoiceFormat tags in the last commit. This will add #759 to adventure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments - also needs an accompanying docs PR.
text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/Tag.java
Outdated
Show resolved
Hide resolved
text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/Formatter.java
Outdated
Show resolved
Hide resolved
text-minimessage/src/main/java/net/kyori/adventure/text/minimessage/tag/resolver/Formatter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (pending docs pr)! Thanks for getting to all the requested changes and such :) Very excited to start using these tags!
It might be a good idea to have a decent way of formatting Numbers and Dates.
Currently I have to use
miniMessage.deserialize("Number is <no>", Placeholder.unparsed("no", String.valueOf(doubleValue)));
Would be nice if it could be smth like
miniMessage.deserialize("Number is <no:'de-DE':'#,00'>, Formatter.formatNumber("no", doubleValue));
where#,00
is the DecimalFormat used to convert the number andde-DE
is the locale tag.Same thing for Formats for Date, Time and both combined.
(miniMessage.deserialize("Date is <date:'YYYY-MM-DD'>", Placeholder.formatDate("date", myDateAsInstant)))
This PR should add those things to Placeholders. It works already fine.
However I want to add fallback values to formatDate and formatNumber. Better display a "bad" formatted message than nothing.
Possible things:
<number:'locale':'format'>
<number:'format'>
<number:'locale'>
<date:'pattern'>
Missing stuff: