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

Core: Floor and ceil in datastorage #2448

Merged
merged 4 commits into from
Nov 24, 2023

Conversation

Ishigh1
Copy link
Contributor

@Ishigh1 Ishigh1 commented Nov 11, 2023

What is this fixing or adding?

Currently it's not doable to divide an integer by another without it becoming a float. This PR adds floor and ceil to the list of possible operations to fix that issue.
I thought about adding an operation to change the type, but that would be either vulnerable to code injection or annoying to do for a niche feature

How was this tested?

I just tested if floor and ceil worked as intended

If this makes graphical changes, please attach screenshots.

@ThePhar
Copy link
Member

ThePhar commented Nov 11, 2023

Rather than a floor_div function, why not add generic floor and ceil operations?

Then you do the "div" operation and finish with a floor operation as you pass in a list of operations to do in order. Then it can also be useful for other operations that may have float results but we want to remove the decimal somehow.

@ThePhar ThePhar added is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. labels Nov 11, 2023
@Ishigh1
Copy link
Contributor Author

Ishigh1 commented Nov 11, 2023

That's also an idea, I'm not against it
I added both of them then will remove either the floor/ceil or div depending on feedback (also I reordered the operations to match the doc because it was bothering me)

@ThePhar
Copy link
Member

ThePhar commented Nov 12, 2023

Yeah, if we're going to add floor and ceil, then we can remove floor_div as it can be done with two DataStorageOperation objects. I'll wait and see if another core dev disagrees however.

@Jarno458
Copy link
Collaborator

I agree separated floor and ceil operations would be best, just like other operations it can be chained.
and we can remove floor_dev

@Jarno458 Jarno458 added the is: documentation Improvements or additions to documentation. label Nov 15, 2023
@Jarno458
Copy link
Collaborator

an oddity would be that floor and ceil dont use the operator value, but the same is true for the default operation

@Ishigh1 Ishigh1 changed the title Core: Floor_div in datastorage Core: Floor and ceil in datastorage Nov 16, 2023
@ThePhar ThePhar merged commit 530e792 into ArchipelagoMW:main Nov 24, 2023
12 checks passed
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: documentation Improvements or additions to documentation. is: enhancement Issues requesting new features or pull requests implementing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants