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

Validate range #725

Merged
merged 10 commits into from
Aug 2, 2023
Merged

Validate range #725

merged 10 commits into from
Aug 2, 2023

Conversation

minimav
Copy link
Contributor

@minimav minimav commented Mar 30, 2023

This implements the validation on the Range param suggested in #698. I tried to match what I could see in other params which had such logic already. A couple of minor things:

  • 'q's in the order error message for a named param was a compromise since %r's results in 'q''s which looks a bit awkward.
  • The order error message could perhaps be tweaked to Range bounds... when validating the bounds rather than Range parameter ... for both bounds and the param value.

Fixes #698

Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @minimav for this well appreciated PR! I have left one inline comment, otherwise it looks good, and I am very pleased that you made the effort to add some tests 👍

param/__init__.py Outdated Show resolved Hide resolved
@minimav
Copy link
Contributor Author

minimav commented Apr 17, 2023

Thanks @maximlt, I have been a little busy and didn't manage to make the change you suggested.

param/__init__.py Outdated Show resolved Hide resolved
@jbednar jbednar added this to the 2.0 milestone May 12, 2023
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!!

Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, let's get this merged!

@maximlt
Copy link
Member

maximlt commented Jul 31, 2023

Yep let me have a last look and merge after that.

@maximlt
Copy link
Member

maximlt commented Aug 2, 2023

Thanks a lot for your contribution @minimav ! This fix is going to be included in the release of Param 2.0.

@maximlt maximlt merged commit c5c642f into holoviz:main Aug 2, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stricter validation for Range
5 participants