Raise NotImplementedError if using wrong class #135
Conversation
Instead of leaving abstract methods without definition when running on the other platform (Unix vs Windows), define them and raise a PlatformMismatchError if called on the wrong platform. This fixes pylint warnings such as Abstract class 'WindowsFileLock' with abstract methods instantiated Abstract class 'UnixFileLock' with abstract methods instantiated Fixes tox-dev#102
There was a problem hiding this comment.
I prefer in this case the class to remain abstract. This is a clearly faulty warning in pylint and the solution should be to change pylint check than modify our code. #102 was closed for a reason: the issue is not with us, is with pylint.
|
I don't see why you'd want the class to remain abstract, regardless of what pylint may or may not think of it. An abstract class is a class for which the client can be expected to provide an implementation, which doesn't make sense in this case — the client isn't supposed to implement a WindowsFileLock if he's running on Unix, so why would you want it to be an abstract class? I took your comment on #102 as an indication that you were willing to consider a solution "that pleases both the static checker + type checker + documentation generation." Wouldn't this work? |
I mean because it's abstract and lacking methods that should be implemented? 😂 I guess our definition of what abstract class differs significantly. In my book an abstract class is a class that shouldn't be created as it does not have some of its required methods defined (for whatever reason). The current design satisfies that requirement. |
I did consider it, but I'm not in love at the moment with the proposal 🤔 |
|
Ah, OK, fair enough. I did consider raising an issue with Pylint instead, but, well... with my interpretation of what an abstract class is, Pylint's behavior seems rather reasonable to me. If an abstract class defines an interface for client code to implement, if that is what we mean by "abstract class," then it should be fair to assume that being abstract or not isn't a runtime property: If there's any branch where the class is abstract, then it's defining an interface, and then you shouldn't use it without subclassing it first. Let me know if there's any way to tweak my proposal to make it more to your liking. |
Instead of defining a custom exception, PlatformMismatchError, use the standard NotImplementedError.
Add tests of UnixFileLock on Windows systems and WindowsFileLock on non-Windows systems, to make sure they are not abstract (but still raise NotImplementedError). This avoids pylint errors in code using filelock. Also added a note to the change log.
for more information, see https://pre-commit.ci
The Python standard library module "inspect" has a function called "isabstract". That is not a spelling mistake. Silence flake8 SC200 warning about "isabstract".
Make a single test, and select lock_type based on sys.platform.
Next minor release, and today's release date in the change log.
To make it a bit clearer, move the preliminary check of abstractness before the instantiation of a lock, so that the creation of the lock and the pytest.raises(NotImplementedError) are adjacent.
vonschultz
changed the title
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Instead of leaving abstract methods without definition when running on the other platform (Unix vs Windows), define them and raise a
NotImplementedErrorif called on the wrong platform.This fixes pylint warnings such as
Fixes #102