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

Allow multiplication in duration input value #2845

Merged
merged 3 commits into from
May 29, 2024
Merged

Allow multiplication in duration input value #2845

merged 3 commits into from
May 29, 2024

Conversation

lukaszachy
Copy link
Collaborator

@lukaszachy lukaszachy commented Apr 10, 2024

To make it easier for adjust the multiplication happens last: No need to pay attention how adjust creates the value.

Only late I realized the input value for multiplication should accept float, so I will modify that later.
For now: does this approach will be accepted? It is preparation for #1843 so "adjust" doesn't need special handling of duration key.

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • modify the json schema
  • mention the version
  • include a release note

@lukaszachy lukaszachy added this to the 1.34 milestone Apr 30, 2024
@lukaszachy lukaszachy added code | utils Various utility functions and classes used across the code specification Metadata specification (core, tests, plans, stories) labels Apr 30, 2024
@happz
Copy link
Collaborator

happz commented May 27, 2024

@lukaszachy do you plan to mention the version and/or include a release note?

@lukaszachy
Copy link
Collaborator Author

@lukaszachy do you plan to mention the version and/or include a release note?

I wasn't but it is a very good point.

@happz
Copy link
Collaborator

happz commented May 27, 2024

@lukaszachy do you plan to mention the version and/or include a release note?

I wasn't but it is a very good point.

  1. those were the last two unchecked boxes on the PR checklist, and 2. this might be a good feature to point out in release notes, so I asked.

@thrix thrix added the ci | full test Pull request is ready for the full test execution label May 28, 2024
@lukaszachy
Copy link
Collaborator Author

ah, seems schema has the regexp as well, I need to change it.

@thrix thrix self-requested a review May 28, 2024 13:47
@happz happz added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label May 29, 2024
To make it easier for adjust the multiplication happens last: No need to
pay attention how adjust creates the value.
@happz happz enabled auto-merge (squash) May 29, 2024 07:07
@happz happz merged commit ae941d1 into main May 29, 2024
19 checks passed
@happz happz deleted the duration-star branch May 29, 2024 10:14
@LecrisUT
Copy link
Contributor

Sorry for commenting late on this closed PR, but I think pint would be a more robust approach:

>>> import pint
>>> pint.Quantity("30 * 4 min")
<Quantity(120, 'minute')>

Some care needs to be made with regards to translating 30m -> 30min, but that can be done by configuring specific pint.UnitRegistry.context and overwriting the default values for meter and such.

@happz
Copy link
Collaborator

happz commented May 29, 2024

Sorry for commenting late on this closed PR, but I think pint would be a more robust approach:

>>> import pint
>>> pint.Quantity("30 * 4 min")
<Quantity(120, 'minute')>

Some care needs to be made with regards to translating 30m -> 30min, but that can be done by configuring specific pint.UnitRegistry.context and overwriting the default values for meter and such.

That's right, I didn't think about using Pint for this task. I'm not sure if it would handle the ordering of operations @lukaszachy implemented though, it does not follow the mathematical ordering of multiplication and addition.

@LecrisUT
Copy link
Contributor

Indeed, but the current one is also a bit hard to follow. It can look good with

    adjust:
        duration+: 1h

but the mathematical alternative is not bad either though:

    adjust:
        duration+: + 1h

I was hoping that pint would fail if the syntax is broken, but alas it does not:

>>> pint.Quantity("1h 40min")
<Quantity(40, 'hour * minute')>
>>> pint.Quantity("* 40min 1h")
Traceback (most recent call last):

At least the first issue is straightforward to verify by adding a check for dimensionality

>>> pint.Quantity("1h 40min").dimensionality["[time]"]
2

happz pushed a commit that referenced this pull request Jun 2, 2024
To make it easier for adjust the multiplication happens last: No need to
pay attention how adjust creates the value.
The-Mule pushed a commit to The-Mule/tmt that referenced this pull request Oct 14, 2024
To make it easier for adjust the multiplication happens last: No need to
pay attention how adjust creates the value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution code | utils Various utility functions and classes used across the code specification Metadata specification (core, tests, plans, stories) status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants