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

Improve Parameter error messages #808

Merged
merged 12 commits into from
Aug 4, 2023
Merged

Improve Parameter error messages #808

merged 12 commits into from
Aug 4, 2023

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Jul 31, 2023

The error messages raised by Parameters are pretty inconsistent and often lack context.

For now with this PR I'm just seeking feedback on a utility I've added that builds a message supposed to prefix most of (if not all) the errors raised by Parameters. I've just adapted the type validation performed by Number to show the error messages emitted in various situations:

image

(I'd hope to get this into the 2.0 release.)

@maximlt maximlt requested a review from philippjfr July 31, 2023 18:16
@maximlt
Copy link
Member Author

maximlt commented Aug 2, 2023

@philippjfr this is ready.

@jbednar
Copy link
Member

jbednar commented Aug 2, 2023

Thanks. Now seems like a good chance to make the error message a bit cleaner. Instead of ", not type <class 'str'>", can it be either ", not <class 'str'>", or ", not type 'str'", (my preference, I think)? I'm a bit confused why it comes out that way right now, as if I do "type('st')" I get "str", not the confusing version.

@maximlt
Copy link
Member Author

maximlt commented Aug 3, 2023

Yes that'd be a good idea! I won't have time to take care of that now though.

@jbednar
Copy link
Member

jbednar commented Aug 3, 2023

Ah, apparently Python only shows the __name__ of the type when used interactively, so it would be f", not '{type(obj).__name__}'" (yielding , not 'str').

@jbednar jbednar added this to the 2.0 milestone Aug 3, 2023
@philippjfr
Copy link
Member

Personally I find , not 'str' a little weird, the single quotes throw me off.

param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
tests/testaddparameter.py Outdated Show resolved Hide resolved
tests/testcolorparameter.py Outdated Show resolved Hide resolved
tests/testdateparam.py Outdated Show resolved Hide resolved
tests/testdateparam.py Outdated Show resolved Hide resolved
tests/testlist.py Outdated Show resolved Hide resolved
tests/testlist.py Outdated Show resolved Hide resolved
tests/testrangeparameter.py Outdated Show resolved Hide resolved
tests/testtupleparam.py Outdated Show resolved Hide resolved
tests/testtupleparam.py Outdated Show resolved Hide resolved
tests/testtupleparam.py Outdated Show resolved Hide resolved
tests/testtupleparam.py Outdated Show resolved Hide resolved
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.

Thanks for the fixes!

tests/testdateparam.py Outdated Show resolved Hide resolved
@jbednar jbednar merged commit 37b82cf into main Aug 4, 2023
@jbednar jbednar deleted the improve_errors branch August 4, 2023 21:37
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.

3 participants