Ticket #7805 (closed: fixed)
Add exclusive bound option to BoundedValidator
Reported by: | Samuel Jackson | Owned by: | Samuel Jackson |
---|---|---|---|
Priority: | major | Milestone: | Release 3.0 |
Component: | Framework | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Tester: | Owen Arnold |
Description
BoundedValidator currently only performs inclusive bound checks. It would be nice to have the option to have it optionally perform exclusive bound checks as well.
Attachments
Change History
comment:2 Changed 7 years ago by Samuel Jackson
Added methods to check if a bound is inclusive.
Refs #7805
Changeset: 288fde02d54b671e82f3a8e2b73f39bd75c80c84
comment:3 Changed 7 years ago by Samuel Jackson
Updated unit test.
Also fix bug with inclusive bounds setting in the process.
Refs #7805
Changeset: da37bb9163370759383b461871cdb18699e496ba
Changed 7 years ago by Samuel Jackson
- Attachment DummyAlg.py added
Python script that could be useful for testing.
comment:5 Changed 7 years ago by Samuel Jackson
- Status changed from inprogress to verify
- Resolution set to fixed
- Milestone changed from Backlog to Release 3.0
To Tester
This feature isn't implemented anywhere in the project yet, so I've supplied a python script that defines a dummy algorithm with both a Float and Int validator for properties. This can be used to check that setting the bounds for a validator are working properly and that the existing functionality hasn't been broken.
I also suggest giving the code and unit tests (both python and c++) a visual check to make sure I haven't made any silly mistakes.
comment:6 Changed 7 years ago by Wenduo Zhou
- Status changed from verify to verifying
- Tester set to Wenduo Zhou
comment:7 Changed 7 years ago by Wenduo Zhou
- Status changed from verifying to reopened
- Resolution fixed deleted
I tested the python algorithm attached.
As I set the 'inclusive' to True, such that self.declareProperty(name='FloatValue',defaultValue=5.0,validator=FloatBoundedValidator(1.0,10.0,True), doc='A Float Value'), 'FloatValue', value 1.0 is not accepted. If I set 'inclusive' to False, 1.0 is allowed.
I think 'inclusive' being True means that the boundary value, for example 1.0, should be an allowed value. So there is something inconsistent in the doc and code.
comment:8 Changed 7 years ago by Samuel Jackson
- Status changed from reopened to inprogress
Change flag from inclusive to exclusive.
Updated documentation appropriately.
Refs #7805
Changeset: 252d8d8f258fd4df80a527b57bb7a407ea8f5d4d
comment:9 Changed 7 years ago by Samuel Jackson
Refs #7805 Missed uppercase letter.
Changeset: 0ecf6a26ba9230f1fb6e184b111b2704c3681c6a
comment:10 Changed 7 years ago by Samuel Jackson
- Status changed from inprogress to verify
- Resolution set to fixed
comment:11 Changed 7 years ago by Owen Arnold
- Status changed from verify to verifying
- Tester changed from Wenduo Zhou to Owen Arnold
comment:12 Changed 7 years ago by Owen Arnold
- Status changed from verifying to reopened
- Resolution fixed deleted
The coding itself looks fine, but the API is open to misuse. See below. Golden rule = An API should be easy to use correctly and very hard to use incorrectly.
v = FloatBoundedValidator(0.0, 1.0, True) print v.isLowerExclusive() print v.isUpperExclusive() v.setLower(3) # Silently sets it to be Inclusive via Exclusive = False on the default parameter value v.setUpper(4) # Silently sets it to be Inclusive via Exclusive = False on the default parameter value # I inteded to simply change the values, but now it's also gone and changed other behaviour to be Inclusive from Exclusive. print v.isLowerExclusive() # Changed! print v.isUpperExclusive() # Changed!
comment:13 Changed 7 years ago by Samuel Jackson
- Status changed from reopened to inprogress
Refs #7805 Fixed issues with bounds.
Changeset: b09d8d1812bbb5a1fd36dd1ffe1a471cb7e60710
comment:14 Changed 7 years ago by Samuel Jackson
- Status changed from inprogress to verify
- Resolution set to fixed
comment:16 Changed 7 years ago by Owen Arnold
- Status changed from verifying to closed
Merge remote-tracking branch 'origin/feature/7805_bounded_validator_exclusive_bounds'
comment:17 Changed 7 years ago by Owen Arnold
The DummyAlgorithm works as I would expect it to. The API is much better.
comment:18 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 8650
Changed BoundedValidator to have optional inclusive bounds argument.
Also updated python export to include optional parameters in the defintion.
Refs #7805
Changeset: c2db842948925227684242292b399d97d8e1cc7f