3.3.0 breaks pylint

[Zahlii]

Since the last update, I am getting the following warnings. I suspect this has to do with the fact that pylint cannot deduce that the correct instance will not be superclassed by ABC (if-else definition of classes).

import filelock

with filelock.FileLock("abc.lock"):
    print("hi")

pylint bla.py 
************* Module bla
bla.py:1:0: C0114: Missing module docstring (missing-module-docstring)
bla.py:3:5: E0110: Abstract class 'WindowsFileLock' with abstract methods instantiated (abstract-class-instantiated)
bla.py:3:5: E0110: Abstract class 'UnixFileLock' with abstract methods instantiated (abstract-class-instantiated)

[gaborbernat]

This is a pylint bug, not ours 👍 Please open an issue there.

[Zahlii]

It may be that pylint is not perfect in this regard, I just think that fixing this in this package is much more straightforward and quicker than trying to get pylint to cater to this corner case.

Two ways I see:
a) Instead of defining the "alternative" and non-fucntional classes here https://github.com/tox-dev/py-filelock/blob/main/src/filelock/_windows.py#L51 , why not raise an error directly? You already check the sys.platform in the init.py, so unless the user imports _windows directly (which he shouldn't, as its private)you have the guarantee that the platform is correct. Same goes for Linux.
b) At least remove the ABC from the alternative implementation. Attempting to go down this code path shouldn't happen anyway (unless importing directly, see above), so I don't think a user who manages to do this kind of import has any benefit from getting a "trying to instantiate class with abstract methods xyz" as opposed to a clear ValueError("Attempting to use windows locks on linux")

[gaborbernat]

Note the import in the root package are not guarded by the if branch, so they're always executed, therefore you cannot raise the error without moving the import.

[gaborbernat]

You'll need to do some mangling with the type checker too, but if you put in a PR to change it and passes the CI I'll review it.

[vladak]

This is sort of unfortunate approach I'd say. I am not going to debate whether this is pylint problem or not. What will likely happen is that those who run pylint on the code using filelock will likely stick to 3.2.0.

[gaborbernat]

You'll need to do some mangling with the type checker too, but if you put in a PR to change it and passes the CI I'll review it.

[tvainika]

This is a pylint bug, not ours

Is there a bug report open now in pylint? I just got hit by this in automated test suite, which didn't have locked version. Just waiting over a weekend now to decide if locking to older version or not.

[gaborbernat]

Instead of defining the "alternative" and non-fucntional classes here https://github.com/tox-dev/py-filelock/blob/main/src/filelock/_windows.py#L51 , why not raise an error directly? You already check the sys.platform in the init.py, so unless the user imports _windows directly (which he shouldn't, as its private)you have the guarantee that the platform is correct. Same goes for Linux.

Seems this cannot be done because unsettles the type checker.

b) At least remove the ABC from the alternative implementation. Attempting to go down this code path shouldn't happen anyway (unless importing directly, see above), so I don't think a user who manages to do this kind of import has any benefit from getting a "trying to instantiate class with abstract methods xyz" as opposed to a clear ValueError("Attempting to use windows locks on linux")

If I remove the ABC then the static checkers complain that class does not implement all abstract methods from the base.

If someone comes up with a solution that pleases both the static checker + type checker + documentation generation I'm still happy to review. In the meantime I'll close it as a pylint bug.

[PhilippSelenium]

@Zahlii Did you report this in the pylint repository? Could we add a link here?

[Zahlii]

Given that this issue can be "fixed" by disabling the inspection per statement I haven't bothered opening it, especially since I am not entirely sure this indeed is a pylint bug.

# pylint: disable=abstract-class-instantiated
with filelock.FileLock(lockfile):
    pass

[vonschultz]

If someone comes up with a solution that pleases both the static checker + type checker + documentation generation I'm still happy to review. In the meantime I'll close it as a pylint bug.

Would it work to have the methods raise e.g. a PlatformMismatchError if they run on the wrong platform? (See suggestion in pull request.)