Coerce dimensionless values of SizeConstraint to values with dimensions

[skycastlelily]

Pull Request Checklist

• implement the feature
• write the documentation
• extend the test coverage
• update the specification
• adjust plugin docstring
• modify the json schema
• mention the version
• include a release note

[skycastlelily]

Updated^^

…

On Thu, Jul 4, 2024 at 10:27 PM Miloš Prchlík ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tmt/hardware.py <#3076 (comment)>: > ) -> T: + if not isinstance( + UNITS(raw_value), + pint.Quantity): # type: ignore[reportUnnecessaryIsInstance,unused-ignore] + raw_value = str(pint.Quantity(raw_value, unit)) I would recommend doing it *after* _from_specification() call, inspecting the returned constraint. Doing it *before* has the disadvantage of various input types, for example, your test will not trigger for memory: "1024", because raw_value would be a string. On the other hand, *after* the call, the value attribute of constraint returned by _from_specification() would always be a Quantity instance, and much safer to work with, with no exceptions. — Reply to this email directly, view it on GitHub <#3076 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFR23HQI3KWDZQHMB6PTELZKVLTXAVCNFSM6AAAAABKIWY2AOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJZGA2TSMZUHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[happz]

@skycastlelily please, add also a unit test, a small one should be more than enough. A simple call to SizeConstraint.from_specification & checking the magnitude and unit of the returned constraint value should work. I would make it parametrized, with raw value and default unit as inputs and magnitude and unit as expected values, because we want to check various combination - number without a default value (bytes are added), number with a default value but not bytes (the default value is used, not bytes), number with unit already included (4 GB) (default value is ignored), and maybe there are others.

[skycastlelily]

Updated^^

…

On Tue, Jul 9, 2024 at 3:15 PM Miloš Prchlík ***@***.***> wrote: @skycastlelily <https://github.com/skycastlelily> please, add also a unit test, a small one should be more than enough. A simple call to SizeConstraint.from_specification & checking the magnitude and unit of the returned constraint value should work. I would make it parametrized, with raw value and default unit as inputs and magnitude and unit as expected values, because we want to check various combination - number without a default value (bytes are added), number with a default value but not bytes (the default value is used, not bytes), number with unit already included (4 GB) (default value is ignored), and maybe there are others. — Reply to this email directly, view it on GitHub <#3076 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFR23AEMWWO3YC6QB7VTB3ZLOEWZAVCNFSM6AAAAABKIWY2AOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJWG44DGMZYGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[skycastlelily]

Updated:)

…

On Thu, Jul 18, 2024 at 2:08 AM Filip Vágner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/unit/test_hardware.py <#3076 (comment)>: > @@ -67,6 +67,26 @@ def test_constraint_name_pattern(value: str, expected: tuple[Any, Any]) -> None: assert match.groups() == expected +_size_constraint_pattern_input = [ + ({'name': 'num_with_default', 'raw_value': '10', 'default_unit': 'GiB'}, + 'num_with_default: == 10 gibibyte'), + ({'name': 'num_without_default', 'raw_value': '1024'}, 'num_without_default: == 1024 byte'), + ({'name': 'num_with_unit', 'raw_value': '10 GiB', 'default_unit': 'GiB'}, I would suggest using a different default_unit to make it clear that the unit from raw_value is being used. ⬇️ Suggested change - ({'name': 'num_with_unit', 'raw_value': '10 GiB', 'default_unit': 'GiB'}, + ({'name': 'num_with_unit', 'raw_value': '10 GiB', 'default_unit': 'MiB'}, ------------------------------ In tmt/hardware.py <#3076 (comment)>: > @@ -507,7 +507,8 @@ def _from_specification( as_quantity: bool = True, as_cast: Optional[Callable[[str], ConstraintValueT]] = None, original_constraint: Optional['Constraint[Any]'] = None, - allowed_operators: Optional[list[Operator]] = None + allowed_operators: Optional[list[Operator]] = None, + default_unit: Optional[Any] = "bytes" Just a minor suggestion. Could you also add a docstring for the default_unit, as every other parameter has one as well? — Reply to this email directly, view it on GitHub <#3076 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKFR23GXJWJ7AUWO4A3HITTZM2XINAVCNFSM6AAAAABKIWY2AOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOBTGY2TCMBVGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[happz]

/packit build

[happz]

Usual suspects broke tests once again, but it's nothing related to the patch. Merging.