Remove redundant resolve_load_version call

[noklam]

Signed-off-by: Nok Chan [email protected]

Description

Related to #1691
Partially related to #1654 but I want to split this out since this is a separate bug.

Development notes

• Remove a redundant resolve_load_version call
• Fix the incorrect test for MatplotlibWriter
• Similar fix for HoloviewsWriter

Note:
MatplotlibWriter doesn't support load at all. The test is likely copy&paste error, it should be a more specific pattern.

    def _load(self) -> NoReturn:
        raise DataSetError(f"Loading not supported for '{self.__class__.__name__}'")

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes

[SajidAlamQB]

Nice catch, LGTM! 👍

[noklam]

This is actually quite interesting - pylint complains this is a useless parent delegation since the load method is exactly the same as AbstractDataSet. But let's address this in the refactoring PR instead. It feels like this is the initial design - thus the load method was necessary but later other changes come in, it become obsolete for a long time without anyone noticing.

[noklam]

More context of this Refactor PR of versioning in 2+ years ago

[merelcht]

Posting the original conversation about this line:

Isn't Lorena's point "The only other thing I can think of is that it acts as an extra protection measure that this gets called/resolved at some point in the load. At the moment it would only be called from _get_load_path, which we encourage users to call in _load but nothing stops them from forgetting to do this or doing something else entirely."
still valid?

[noklam]

Added some notes here for reference, I have spoken to @lorenabalan to get her perspective. Thanks a lot @lorenabalan for explaining! ⭐️

@lorenabalan point

I personally wouldn’t remove it without having a refactoring plan in place

• The function itself is not necessary - it signals that it does something more than the AbstractDataSet.
• Users need to know a lot about the private API - and it's easy to get wrong. So singal this in the public method helps.
• you can absolutely remove the resolve_load_version from load, but only because we know it’s called further down the chain.

@noklam view
I agree it’s nice to signal that - but I also think we agree that in general, the AbstractVersionDataSet isn’t a nice abstract class - as a result people need to copy a lot of functions. The resolve_load_version alone is not enough - in reality you almost always need this load_path = get_filepath_str(self._get_load_path(), self._protocol), so I am not convinced to keep this.

In terms of implementation, it is not doing the correct thing. The 2 failing test cases are a signal of this, they are not allowed to be loaded so it should throw a DataSetError, but because of the extra resolve_load_version call, it was throwing the VersionNotFoundError instead.
This makes me more lean to remove it now, as this is incorrect and I think it overwhelms the benefit of signaling what's the right thing to do. In reality - people are copy&pasting already

@MerelTheisenQB 's view

• Lean to remove it now - Especially the tests are a strong sign that this indeed wasn’t the right behavior.
• I think the refactoring of the load_version logic is a good opportunity to improve the call flow and in that way still make it clear somehow that AbstractVersionDataSet is doing more than AbstractDataSet

[noklam]

More context in this Refactor PR of versioning in 2+ years ago

[merelcht]

Thanks for investigating and explaining what was going on here @noklam ! ⭐ It's a great improvement that the holoviews and matplotlib datasets now return the correct error message.

[merelcht]

Maybe we should rename this and the other test to test_load_not_supported instead.

[noklam]

Agree, this will be more accurate - will do