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

⚒️ Fix ExprFormatTime CCE + Support non-literals #4664

Merged
merged 23 commits into from
Jul 18, 2022

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented Mar 15, 2022

Description

  • Fixed CCE
  • Supported the use of non-literals as custom format such as broadcast now formatted as {_someVar}
  • Changed Class name and Doc name Date instead of Time
  • Added new pattern

Before PR (CCE)

on chat:
	set {_test} to "HH:mm"
	broadcast now formatted as {_test}

image

After PR

image


Target Minecraft Versions: Any
Requirements: None
Related Issues: None

@AyhamAl-Ali AyhamAl-Ali marked this pull request as draft March 15, 2022 13:37
@AyhamAl-Ali AyhamAl-Ali added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Mar 15, 2022
@Pikachu920
Copy link
Member

instead of requiring a use of literals here, can we just create the format in get if expr[1] is not a literal?

@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented Mar 15, 2022

instead of requiring a use of literals here, can we just create the format in get if expr[1] is not a literal?

It's only there to give skript errors on init if the pattern is incorrect and an exception was thrown tho in this class it wasn't throwing exceptions as in #4663

I will be doing some tests on this to try if I can make it throw an exception if so will have to use try/catch otherwise I think we can do what you said

EDIT:
I was able to trigger it with the character i
!broadcast now formatted with "i"
image

@TPGamesNL
Copy link
Member

@Pikachu920 The reason it was initially required to be a literal is because that way the format can be checked for errors. If we were to parse it as a format at runtime, we could either error at runtime (which I don't think is great, people don't look at console often), or exit silently (which isn't helpful to the user).
Alternatively, we could check if it's a literal (or simple VariableString) and then error if the format is invalid. What would be the best option here?

@Pikachu920
Copy link
Member

@Pikachu920 The reason it was initially required to be a literal is because that way the format can be checked for errors. If we were to parse it as a format at runtime, we could either error at runtime (which I don't think is great, people don't look at console often), or exit silently (which isn't helpful to the user).
Alternatively, we could check if it's a literal (or simple VariableString) and then error if the format is invalid. What would be the best option here?

I think that returning null silently is the best option. It's nice to have the format checking for literals, but I don't think limiting to only literals is necessary.

@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented Mar 15, 2022

I think that returning null silently is the best option. It's nice to have the format checking for literals, but I don't think limiting to only literals is necessary.

Though should we check in init if exprs[1] is literal and simple VariableString then we can check for format exceptions otherwise will leave it to runtime and silent the errors, what do you think?

Also, what about leaving warning at runtime for format errors so that at least if a good skripter checks the console would know what's going on/why it isn't working (tho it might spam their console but that's a user error)

@Pikachu920
Copy link
Member

I think that returning null silently is the best option. It's nice to have the format checking for literals, but I don't think limiting to only literals is necessary.

Though should we check in init if exprs[1] is literal and simple VariableString then we can check for format exceptions otherwise will leave it to runtime and silent the errors, what do you think?

Also, what about leaving warning at runtime for format errors so that at least if a good skripter checks the console would know what's going on/why it isn't working (tho it might spam their console but that's a user error)

Totally agree. Check for format exceptions when possible, otherwise silently ignore.

@AyhamAl-Ali AyhamAl-Ali changed the title ⚒️ Fix ExprFormatTime CCE ⚒️ Fix ExprFormatTime CCE + Support non-literals Mar 15, 2022
@AyhamAl-Ali AyhamAl-Ali added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Mar 15, 2022
@AyhamAl-Ali AyhamAl-Ali marked this pull request as ready for review March 15, 2022 22:02
@AyhamAl-Ali AyhamAl-Ali requested a review from Pikachu920 March 16, 2022 15:22
@AyhamAl-Ali AyhamAl-Ali requested a review from TPGamesNL March 16, 2022 21:12
@AyhamAl-Ali AyhamAl-Ali requested a review from TPGamesNL June 24, 2022 08:51
AyhamAl-Ali and others added 2 commits July 17, 2022 13:37
Co-authored-by: TPGamesNL <29547183+TPGamesNL@users.noreply.github.com>
@AyhamAl-Ali AyhamAl-Ali requested a review from TPGamesNL July 17, 2022 11:11
@TPGamesNL TPGamesNL merged commit 58081ab into SkriptLang:master Jul 18, 2022
AyhamAl-Ali added a commit to AyhamAl-Ali/Skript that referenced this pull request Apr 21, 2023
ALOBugTea pushed a commit to ALOBugTea/Skript that referenced this pull request Sep 29, 2024
sovdeeth pushed a commit that referenced this pull request Oct 31, 2024
* 🚀 Add number format expression

* Little changes

* Prevent variables usage

* Support variables as custom format

* Make it return null if format is invalid

* Add test script

* Update test script

* Update test script

* Changes

* Fix class to use same technique as #4664

* Apply suggestions from code review

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>

* Add helpful comments and use lambda

* Update src/main/java/ch/njol/skript/expressions/ExprFormatNumber.java

Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>

* Change expression to function

* Remove unused imports

* Refactoring and merge base, updating examples

* return null instead of empty string

* Update examples

* Warn instead of erroring, but probably better to remove

* Address reviews

* better variable naming

* init commit

* update file name

---------

Co-authored-by: Ayham Al-Ali <alali_ayham@yahoo.com>
Co-authored-by: Ayham Al-Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
Co-authored-by: Moderocky <admin@moderocky.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants