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

Add underscore formatting in numbers #7030

Merged
merged 15 commits into from
Oct 13, 2024

Conversation

Efnilite
Copy link
Member

@Efnilite Efnilite commented Sep 2, 2024

Description

Resolves #7015. Adds underscore support for formatting in numbers.


Target Minecraft Versions: any
Requirements: none
Related Issues: #7015

@Efnilite Efnilite changed the title Underscores in numbers Add underscore formatting in numbers Sep 2, 2024
@Fusezion
Copy link
Contributor

Fusezion commented Sep 2, 2024

I don't believe we need a regression test currently but an actual test added, this isn't so much resolving a bug as much as adding an enhancement to numbers.

@Efnilite
Copy link
Member Author

Efnilite commented Sep 2, 2024

I don't believe we need a regression test currently but an actual test added, this isn't so much resolving a bug as much as adding an enhancement to numbers.

Should I put it in the misc folder then?

@Fusezion
Copy link
Contributor

Fusezion commented Sep 2, 2024

Yeah I think misc is fair as this isn't syntax related

@Asleeepp
Copy link
Contributor

Asleeepp commented Sep 2, 2024

Kenzie mentioned something about adding a way to detect if it was a valid number, something like ____1___ shouldn't be valid

heres the discord conversation if you're curious: here

@UnderscoreTud
Copy link
Member

Kenzie mentioned something about adding a way to detect if it was a valid number, something like _1 shouldn't be valid

heres the discord conversation if you're curious: here

That already isn't valid, but my main concern is how you can have adjacent underscores, for example 1___0 is valid when I don't think it makes that much sense.

@Efnilite
Copy link
Member Author

Efnilite commented Sep 2, 2024

Kenzie mentioned something about adding a way to detect if it was a valid number, something like _1 shouldn't be valid

heres the discord conversation if you're curious: here

This has already been addressed in the last lines of the test. Parsing the numbers starting and/or ending with _ seems to resolve to none, as these numbers don't match the updated regex. If anything else should be changed, please lmk

@Efnilite
Copy link
Member Author

Efnilite commented Sep 2, 2024

Kenzie mentioned something about adding a way to detect if it was a valid number, something like _1 shouldn't be valid
heres the discord conversation if you're curious: here

That already isn't valid, but my main concern is how you can have adjacent underscores, for example 1___0 is valid when I don't think it makes that much sense.

Personally I agree that having multiple _ is a little silly but someone might have a genuine use case for it, like maybe someone prefers 1__000__000 over 1_000_000. Java also support this.

I don't think this reason should axe this entire PR. It would be possible to limit it to just one _ if more people think it's a good idea.

@Asleeepp
Copy link
Contributor

Asleeepp commented Sep 2, 2024

Kenzie mentioned something about adding a way to detect if it was a valid number, something like _1 shouldn't be valid
heres the discord conversation if you're curious: here

That already isn't valid, but my main concern is how you can have adjacent underscores, for example 1___0 is valid when I don't think it makes that much sense.

Personally I agree that having multiple _ is a little silly but someone might have a genuine use case for it, like maybe someone prefers 1__000__000 over 1_000_000. Java also support this.

I don't think this reason should axe this entire PR. It would be possible to limit it to just one _ if more people think it's a good idea.

then i think that it would be better to make it consistent across the whole number
someone cant do 1__000_000 for example
though maybe there should also be a limit on how many underscores you can use

@UnderscoreTud
Copy link
Member

UnderscoreTud commented Sep 2, 2024

Kenzie mentioned something about adding a way to detect if it was a valid number, something like _1 shouldn't be valid
heres the discord conversation if you're curious: here

That already isn't valid, but my main concern is how you can have adjacent underscores, for example 1___0 is valid when I don't think it makes that much sense.

Personally I agree that having multiple _ is a little silly but someone might have a genuine use case for it, like maybe someone prefers 1__000__000 over 1_000_000. Java also support this.

I don't think this reason should axe this entire PR. It would be possible to limit it to just one _ if more people think it's a good idea.

@APickledWalrus and I talked about it on discord and he also thought the same thing (that it shouldn't be allowed), but we can wait and see what the other members think

@sovdeeth
Copy link
Member

sovdeeth commented Sep 2, 2024

oh yes i definitely don't think it you should axe this pr
but i do agree that only 1 underscore makes sense and should be the requirement

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 2, 2024
@Efnilite
Copy link
Member Author

Efnilite commented Sep 2, 2024

Aight, changed it to at most one and added tests for making sure that having multiple isn't supported

Co-authored-by: Asleepp <119438940+Asleeepp@users.noreply.github.com>
@Efnilite Efnilite requested a review from Asleeepp September 3, 2024 12:30
Copy link
Contributor

@Asleeepp Asleeepp left a comment

Choose a reason for hiding this comment

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

since you requested my review (my approval does nothing haha)

@Efnilite Efnilite added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Sep 16, 2024
@sovdeeth sovdeeth merged commit 97055a2 into SkriptLang:dev/feature Oct 13, 2024
5 checks passed
@Efnilite Efnilite deleted the underscores branch November 14, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants