-
Notifications
You must be signed in to change notification settings - Fork 47
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
decorator hash_library kwarg option precedence #154
Comments
I'm happy to take this on, if this is the case 👍 |
I am not sure about this one. It seems to me that having the CLI override the one in the code makes more sense, as it would let a user specify a different test hash lib or something? The fact it's inconsistent is more problematic. I am up for input from others here, @ConorMacBride @astrofrog |
It would be more typical for a CLI argument to override the configured value. However, in this case a different So each test, in theory, could use a different hash library, and overriding the configured value would break this. That said, I think using multiple hash libraries is not tested and therefore not encouraged. If you think we should support a different hash library and baseline directory for each test then I think kwarg should take priority. But if we are to require the same library and directory, I think the CLI argument should take priority. |
This feels like it is walking into a bigger discussion including some of the points raised in #149. At the moment our API is very flexible in terms of what we test against, defined per-test in Python you can do pretty much anything you like based on installed package versions etc. I think I would be pro reducing that complexity a little, and that includes one baseline_dir / hash library per test run. It would make it easier to configure (you could set some config option in That being said, we have provided the per-decorator argument since this package existed (right @astrofrog?) and there are people using it, so any change from that would have to be slow and throw many-a-warning. |
I guess what would be ideal here is aiming for a consistent behaviour across If that's the case, are there more such changes that can be part of such a major release? |
For example, if #150 lands, would you guys consider making If so, then that would be a breaking change in behaviour, and that's a suitable change for a major release... 🤔 |
I agree consistency is a good goal. Planning for a major breaking release also sounds like a good idea. I not sure if I will have the time to really sit down and audit the API, but if someone else wants to do it that would be great :D |
Cool, I'll scrape through the code and attempt to summarise the current state of the API, then we can roll from there 👍 |
I've reviewed the API in #198 and in #199, and have made some suggestions for changes. Suggested changes relevant to this issue are:
For the two deprecations, I don't think we should go out of our way to prevent people doing these but we should detect these cases and warn the user that it is not supported. I do think we should support multiple baseline images directories as it can be nice to have a separate baseline image directory within each test module, and this is actually what I think it is useful to keep the Please let me know if you have any thoughts, either here or in #198 |
Within the
plugin.ImageComparison.compare_image_to_hash_library
method, should it not be the case that thempl_image_compare
markerhash_library
kwarg option should have precedence over the--mpl-hash-library
CLI argument?pytest-mpl/pytest_mpl/plugin.py
Lines 520 to 521 in e387618
At the moment this isn't the case, however a similar precedence is supported for the
baseline_dir
decorator option e.g.,The text was updated successfully, but these errors were encountered: