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

Consider warnings as exceptions in the test suite #738

Merged
merged 12 commits into from
Jun 27, 2023

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Apr 12, 2023

Fixes #201

@maximlt maximlt marked this pull request as draft April 12, 2023 17:44
@maximlt maximlt marked this pull request as ready for review June 21, 2023 20:49
@maximlt
Copy link
Member Author

maximlt commented Jun 21, 2023

@hoxbro I'm interested in your opinion on this one! I really wasn't sure about creating specific warning types and spent some time thinking about that. I saw that indeed some libraries implement custom warnings that are usually just subclasses of the builtin warnings. Notably Bokeh, which is maybe where you saw it in the first place? I saw two advantages of having these specific Param warnings:

  • It allows other libraries to more easily filter these warnings, as you've started to do in GeoViews. I haven't (yet?) put any of these warnings in the __init__.py module as my goal is to clean that namespace and not add to it. So right now you'd turn these warnings into errors with this filter: error::param._utils.ParamWarning. Do you think that ParamWarning should really be exposed in __init__.py? Ideally our test suites (e.g. Panel, HoloViews) would turn all warnings into errors so we wouldn't need to filter these warnings, so my idea is that using the ugly filter above is fine as it's meant to be temporary. Oh and yes I've created a base warning ParamWarning so that other libraries can just set up one filter.
  • It improves the warning messages by giving them some more context as the warning name precedes the warning message (ush this temporary file path, yuk):
image

The disadvantage is that it is more code to maintain of course , which is why I'm interested in keeping it as private as possible, if that's sensible.

@hoxbro
Copy link
Member

hoxbro commented Jun 27, 2023

Notably Bokeh, which is maybe where you saw it in the first place?

Correct, I chose this because I saw it in Bokeh (and liked the approach).

My reason for exposing the warnings in __init__.py is to have them easily accessible if you want to mute them, with that being in the pytest config file or with warnings.filterwarnings. This is especially relevant if you are not familiar with the library itself. Having to dig through the code and find the place with the warning is annoying (or maybe I'm doing it wrong...)

My belief is even though the code itself is in a private module, the warning itself is public because we expose it to our users, and to a certain degree, they have to interact with it either by looking at the red warning or by muting it.

... as the warning name precedes the warning message (ush this temporary file path, yuk):

Yeah, I'm not a fan of that either, but that is how Jupyter works. It is much nicer if the warning is emitted for a Python file, which easily shows your the file and line where the warning occurs.

@maximlt
Copy link
Member Author

maximlt commented Jun 27, 2023

Thanks for your input!

I will keep things as is for two reasons:

  • I think the main reason for us having to provide these specific warnings are to turn them into errors in our test suites, while what we should do is set up our test suites to fail on any warning and ignore those we deem are fine to ignore :) So I see that as an internal hack that we may want to remove one day. Not exposing it more doesn't prevent people from what they're usually doing when filtering a warning, using some regex and module location (as in https://github.com/holoviz/holoviews/blob/8df58d3b485a38236f979f21c2adc9b2a00fa6a0/pyproject.toml#L12-L28), since these warnings are subclasses of Python warnings.
  • I hope that in Param 2.x we'll sit and figure out whether the logging system Param implements, that is mostly used to do warning, is actually needed and if instead we shouldn't rely on standard warnings. Because of that, I don't really want to set a too strong precedent with how Param handles warnings.

@maximlt maximlt merged commit b96169f into main Jun 27, 2023
@maximlt maximlt deleted the test_warnings_as_exceptions branch June 27, 2023 15:08
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.

Enable warnings as errors for param's test suite
2 participants