Pre-Commit compatibillity issue with PYTHONPATH

[niall-byrne]

I'm seeing some behaviour that I wanted to make sure I was identifying correctly.
This is specifically in relation to the --check-overidden option.

There appears to be a variance between the CLI usage and when using the pre-commit hook.

This example will pass CLI checks, but will fail a pre-commit run:

pyproject.toml

[tool.docsig]
check-class_constructor = true
check-dunders = false
check-overridden = false
check-protected = false

folder/bases/base_class.py

class BaseClass:
  """My base class."""

  def method(self, arg) -> None:
    """Does something.

    :param arg: some argument
    """
    return None

folder/implementation1.py

from .bases.base_class import BaseClass

class Implementation(BaseClass):
  """My implementation."""

  def method(self, arg) -> None:
    """Does something."""
    return None

I can run:
docsig folder/implementation1.py

This is no problem.

pre-commit run docsig --all-files

This fails.

After playing around with this a bit, it seems relative imports are the problem.
If I instead change this to a fully qualified import path, it will work consistently.

So this led me to trying:

PYTHONPATH=$PWD pre-commit run docsig --all-files

Which works fine.
I am hesitant to suggest a change at this point, as I haven't looked at the codebase yet.
Do you have any hunches as to how this might be mitigated?

Environment:

• Python version: Python 3.9.16
• docsig version: 0.65.0

[niall-byrne]

I can sort of mitigate it with:

  - repo: https://github.com/jshwi/docsig
    rev: v0.65.0
    hooks:
      - id: docsig
        entry: bash -c 'PYTHONPATH=$PWD docsig $@'
        require_serial: true

[jshwi]

Hi @niall-byrne

Thanks for opening this issue

I am actually aware of this issue, when I came across this the first time it took me a while to work it out, sorry if took you a bit of time too

The inference for check overridden is provided by pylint's astoid, and is explained in the following pylint documentation: https://pylint.pycqa.org/en/stable/user_guide/installation/pre-commit-integration.html

I use it like the following, and therefore am unable to use it with pre-commit's CI, and so have to use it with my github actions

# $ cat .pre-commit-config.yaml
repos:
  - repo: local
    hooks:
      # local so package can be imported to lint
      - id: lint
        name: lint
        entry: make .make/lint
        language: system
        pass_filenames: false

# $ cat Makefile
#: lint code
.make/lint: $(VENV) $(PYTHON_FILES)
	@$(POETRY) run pylint --output-format=colorized $(PYTHON_FILES)
	@$(POETRY) run docsig $(PYTHON_FILES)
	@mkdir -p $(@D)
	@touch $@

The main issue, I think, is poor documentation and --check-overidden won't work when using the hook provided in this package for the pre-commit package manager

As, unlike with pylint, asteroid is not essential for the majority of this package I thought maybe a warning would help, or an error when using --check-overidden with pre-commit, but I never got round to it. What do you think?

Cheers,
Stephen

[niall-byrne]

Thanks for taking a look,

After digging further I'm finding deeply nested relative imports still don't work as expected. I don't mind making them fully qualified, as docsig is very useful, but it might make your tool more widely applicable if this wasn't a nuisance.

It might be worth looking at the solution detailed in here before deciding on anything.

The local hooks thing is definitely a solution for setting the initial PYTHONPATH, it's just unfortunate that it would require either adding docsig as a dependency or manually managing another virtual-environment. I guess that's what makes pre-commit so nice to use.

I'll try taking a look at the issue referenced above to see if there is a solution that can be used.
It appears you are parsing the loaded file here, so any additional context to help astroid would also be passed there as well. It's worth examining, but may not work in all cases.

[niall-byrne]

@jshwi

I have prepared a PR, that's still in progress: #530
This solves the relative path problem by giving ast enough context to figure out where the imports are coming from.

The MAJORITY of tests now pass, but there is a minority that are failing due to differences in the reporting output.

Do you mind having a look?
The side-effect of feeding ast the additional context is that it mangles file names for reporting.

What would be the desired output format? It might be easier to modify them all so there is a consistent naming scheme.

[niall-byrne]

I think I sorted out the reporting issue.

[jshwi]

Hey @niall-byrne
Thanks for submitting the PR, been a busy week but I'll try to review it tomorrow, sorry for the delay
Speak to you then!
Cheers
Stephen

[jshwi]

Hey @niall-byrne
I found enough time to be able to trigger the build and have a look through this PR
There's a sneaky doctest that went off in the action
Everything looks great otherwise. One tiny change and I'll be happy to merge. I dropped a review on the PR
Thanks!
Stephen

[niall-byrne]

@jshwi

I altered the report logic slightly, and am able to run the doctests locally now.
If there are no other problems, we should be ok!

[jshwi]

awesome work @niall-byrne , thank you!