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

[PT Run] Calculator: rand() return float instead of int #17301

Open
1 task
htcfreek opened this issue Mar 27, 2022 · 28 comments
Open
1 task

[PT Run] Calculator: rand() return float instead of int #17301

htcfreek opened this issue Mar 27, 2022 · 28 comments
Assignees
Labels
External Dependency This bug or feature isn't resolved, but it's following an external work item. Idea-Enhancement New feature or request on an existing product Product-PowerToys Run Improved app launch PT Run (Win+R) Window Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Run-Plugin Things that relate with PowerToys Run's plugin interface

Comments

@htcfreek
Copy link
Collaborator

htcfreek commented Mar 27, 2022

Microsoft PowerToys version

0.56.2

Running as admin

  • Yes

Area(s) with issue?

PowerToys Run

Steps to reproduce

type rand().

✔️ Expected Behavior

An integer is returned.

❌ Actual Behavior

image

Other Software

No response

@htcfreek htcfreek added Issue-Bug Something isn't working Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Mar 27, 2022
@htcfreek
Copy link
Collaborator Author

duplicate of #17300

@htcfreek htcfreek added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. Run-Plugin Things that relate with PowerToys Run's plugin interface Run-Plugin Manager Issues with the PowerToys Run plugin manager and removed Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. labels Mar 27, 2022
@htcfreek
Copy link
Collaborator Author

htcfreek commented Mar 27, 2022

Sorry I closed this mistakenly. It isn't a duplicate.

@htcfreek htcfreek reopened this Mar 27, 2022
@htcfreek htcfreek added Product-PowerToys Run Improved app launch PT Run (Win+R) Window and removed Run-Plugin Manager Issues with the PowerToys Run plugin manager labels Mar 27, 2022
@jaimecbernardo
Copy link
Collaborator

I think returning a number between 0 and 1 is pretty common for random functions.
Seems to be working as expected by the engine's documentation as well: https://github.com/FlorianRappl/Mages/blob/6e283ae40f9fb21137a3e339778629563cbda719/doc/functions.md#generate-single-random-number

@htcfreek
Copy link
Collaborator Author

Hmmm. Didn't know that.

@htcfreek
Copy link
Collaborator Author

I think we should have a second operation to return it as int with a custom count of digits. Something like ranint(5).

@jaimecbernardo jaimecbernardo added Idea-Enhancement New feature or request on an existing product and removed Issue-Bug Something isn't working Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Mar 28, 2022
@jaimecbernardo
Copy link
Collaborator

Marking it as an enhancement. Though I wonder if it's possible before changing engine.

@htcfreek
Copy link
Collaborator Author

Marking it as an enhancement. Though I wonder if it's possible before changing engine.

It might be a way to only cut 0, from the result in a first step.

@htcfreek htcfreek changed the title [PT Run] rand return float instead of int [PT Run] Calculator: rand() return float instead of int Mar 28, 2022
@Jay-o-Way
Copy link
Collaborator

Jay-o-Way commented Mar 31, 2022

Both integer and floating are welcome. Sould use 2 different keywords, like @htcfreek mentioned RandInt. --> Rename this issue?

I think we should have a second operation to return it as int with a custom count of digits. Something like ranint(5).

RandInt(5) is also used/interpreted for an integer between 0 and 5.
Decimal variable might be added to the other one: rand(5) => 0,10125.

@jaimecbernardo
Copy link
Collaborator

I think this needs to be done on the engine we're using though, which is an open source Calculator Engine. Or be referenced in the issue about changing calculator engine as a requirement.

@jaimecbernardo
Copy link
Collaborator

For reference, here's the calculator engine:
https://www.nuget.org/packages/Mages/
https://github.com/FlorianRappl/Mages/

@jaimecbernardo
Copy link
Collaborator

@Jay-o-Way , can you please link to it?

@htcfreek
Copy link
Collaborator Author

htcfreek commented Apr 4, 2022

@Jay-o-Way , can you please link to it?

@jaimecbernardo
The pr is closed because it conflicts with my planned changes. But the hotfixes had higher prio for me. I will cover this in me pr that hopefully gets ready this week. ( Then I ping you both.)

@GhostVaibhav
Copy link
Contributor

Hi @htcfreek, should we close this issue? Or, do we still need a function for generating random integers?

@htcfreek
Copy link
Collaborator Author

@GhostVaibhav
It still does not exist. Do you like to implement it?

@GhostVaibhav
Copy link
Contributor

@htcfreek Count me in 👍🏻

@htcfreek htcfreek added the Status-In progress This issue or work-item is under development label Jun 27, 2024
@GhostVaibhav
Copy link
Contributor

GhostVaibhav commented Jun 27, 2024

Hi @htcfreek, some doubts. Would it be easier if rather than handling this request on our end, we handled it on Mages' end? What I meant to say is should I add randInt() implementation on Mages and update the repo's version here, or should I handle it here itself?

@htcfreek
Copy link
Collaborator Author

Hi @htcfreek, some doubts. Would it be easier if rather than handling this request on our end, we handled it on Mages' end? What I meant to say is should I add randInt() implementation on Mages and update the repo's version here, or should I handle it here itself?

The faster solution would be handle in PT Run.

But you can ask to implement it in mages. Not sure if it will get in there.

@GhostVaibhav
Copy link
Contributor

GhostVaibhav commented Jun 27, 2024

The PT Run is a very hacky way of getting this done. What I was thinking,

  1. Match the number inside randInt(342), in this 342 would be matched. Also, replace it with an empty string.
  2. Replace randInt() with "Round($matchedNumber * rand())"

Does this sound good?

@htcfreek
Copy link
Collaborator Author

After thinking again we should first open an issue an maybe a PR on mages repository as this would be the correct way. If the issue/PR not gets accepted, we can implement it in our plugin using a hacky way.

@GhostVaibhav
Copy link
Contributor

Hi @htcfreek, I have raised a PR on the Mages repo. Links to follow -

But, I have one concern, it's listed under the v3.0.0 milestone and so are the 3 other issues. It would take additional time to get them fixed. So, they are inadvertently blocking the release of this feature. Any suggestions?

@htcfreek htcfreek added the External Dependency This bug or feature isn't resolved, but it's following an external work item. label Jun 28, 2024
@htcfreek
Copy link
Collaborator Author

But, I have one concern, it's listed under the v3.0.0 milestone and so are the 3 other issues. It would take additional time to get them fixed. So, they are inadvertently blocking the release of this feature. Any suggestions?

That's what I had feared.

We could asked the repository maintainer if it is possible to backport the feature into v2.x release.

@GhostVaibhav
Copy link
Contributor

Hi @htcfreek, good news. The PR got merged just now. What should be the next course of action? Should we ping the maintainer for a release?

@htcfreek
Copy link
Collaborator Author

htcfreek commented Jun 28, 2024

Hi @htcfreek, good news. The PR got merged just now. What should be the next course of action?

Great news.

Should we ping the maintainer for a release?

Yes, I think we can friendly ask for a time plan. I would reference to the PowerToys repository.

@NoPlagiarism
Copy link

V3.0.0 is out

@GhostVaibhav
Copy link
Contributor

Hi @htcfreek, the latest version of Mages has been released. Included with it is the new randi() function. I'm a little out of touch with the current repo. Can you take it from here?

@htcfreek
Copy link
Collaborator Author

@GhostVaibhav
Is it correct that we only have to update the mages package version and documentation?

@GhostVaibhav
Copy link
Contributor

@htcfreek
Correct

@htcfreek htcfreek self-assigned this Dec 24, 2024
@htcfreek
Copy link
Collaborator Author

@GhostVaibhav
Can take a look in the next days.

@jaimecbernardo jaimecbernardo added Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed Status-In progress This issue or work-item is under development labels Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Dependency This bug or feature isn't resolved, but it's following an external work item. Idea-Enhancement New feature or request on an existing product Product-PowerToys Run Improved app launch PT Run (Win+R) Window Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Run-Plugin Things that relate with PowerToys Run's plugin interface
Projects
Status: No status
Development

No branches or pull requests

5 participants