interrogate icon indicating copy to clipboard operation
interrogate copied to clipboard

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

Open zackyancey opened this issue 2 years ago • 6 comments

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)

  • [X] Changes are covered by unit tests (no major decrease in code coverage %).
  • [X] All tests pass.
  • [X] Docstring coverage is 100% via tox -e docs or interrogate -c pyproject.toml (I mean, we should set a good example :smile:).
  • [X] Updates to documentation:
    • [X] Document any relevant additions/changes in README.rst.
    • [X] Manually update both the README.rst and docs/index.rst for any new/changed CLI flags.
    • [X] Any changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in the project's __init__.py file.

Release note

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

zackyancey avatar Sep 26 '23 15:09 zackyancey

@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!

ErwinJunge avatar Sep 26 '23 19:09 ErwinJunge

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?

zackyancey avatar Sep 27 '23 14:09 zackyancey

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.

hynek avatar Sep 27 '23 14:09 hynek

That did it, thanks!

zackyancey avatar Sep 27 '23 15:09 zackyancey

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

vpoulailleau avatar Nov 27 '23 12:11 vpoulailleau

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?

zackyancey avatar Nov 27 '23 16:11 zackyancey

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

econchick avatar Apr 06 '24 21:04 econchick