econchick / interrogate

Explain yourself! Interrogate a codebase for docstring coverage.
https://interrogate.readthedocs.io
MIT License
575 stars 44 forks source link

Fix windows tests for #98: ignoring @overload decorated functions #160

Closed zackyancey closed 6 months ago

zackyancey commented 1 year ago

Description

This is just the changes by @ErwinJunge in #98, with the expected results for the windows tests updated.

(let me know if I should be somehow adding this to #98 instead, I'm not sure how I'd do that. Or @ErwinJunge if you just want to add these to your branch that would work too).

Motivation and Context

Allows #98 to be merged, which fixes #97

Have you tested this? If so, how?

I've run the tests put in by @ErwinJunge on linux and windows, and run the changes against my own code.

Checklist for PR author(s)

Release note

Add ``--ignore-overloaded-functions`` flag to ignore overload decorators (`#97 <https://github.com/econchick/interrogate/issues/97>`_).
ErwinJunge commented 1 year ago

@zackyancey thanks so much for taking the time to fix the windows part of the tests :bow:

I'm fine with merging this instead of #98, since it contains my singular commit from #98 anyways :)

However, now the pre-commit has started failing on missing interrogate for some reason? :thinking:

Anyways, you have my full support, let's get this merged!

zackyancey commented 1 year ago

Hm, it looks like the same thing is happening on master and on other PRs newer than a couple weeks old. It seems like it might be related to the recent move to pre-commit.ci -- @hynek @econchick any idea what could be causing the precommit to not find interrogate?

hynek commented 1 year ago

Yes it's because the local install step doesn't work there. It should be replaced by a normal pre-commit installation and add dog-fooding to Tox.

zackyancey commented 1 year ago

That did it, thanks!

vpoulailleau commented 10 months ago

Any news on this? (The merge would be perfect for me :wink:)

zackyancey commented 10 months ago

As far as I can tell the tests should be passing now -- I think all that's needed to get it merged is review/approval. @econchick am I missing anything here?

econchick commented 6 months ago

Thanks for your help! I cherry picked your commits and included them in #167 ❤️