econchick / interrogate

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

Add `--ignore-overloaded-functions` flag to ignore overload decorators #98

Closed ErwinJunge closed 6 months ago

ErwinJunge commented 2 years ago

Hey, I just made a Pull Request!

Description

Add --ignore-overloaded-functions flag to ignore overload decorators

Motivation and Context

Fixes #97

Have you tested this? If so, how?

I have included unittests and ran interrogate against a codebase with overloads.

Checklist for PR author(s)

Release note

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

dang! thanks for the contribution @ErwinJunge ! I'm wrapping up a bunch of work stuff, but I'll review this over the next few days. Thanks!

ErwinJunge commented 2 years ago

Apparently I failed at running black :facepalm:

I've fixed the codestyle and forcepushed a new commit.

ErwinJunge commented 2 years ago

dang! thanks for the contribution @ErwinJunge ! I'm wrapping up a bunch of work stuff, but I'll review this over the next few days. Thanks!

For me this is work stuff :) I'm finishing up my week now and will pick this back up monday if there's any feedback.

ErwinJunge commented 2 years ago

@econchick friendly ping on the review :)

ErwinJunge commented 2 years ago

Hi @econchick , I'm sorry to hear about the covid and I hope you had a full recovery!

You're welcome for the PR. The test suite could indeed use some work, there were a lot of unrelated testing changes required to add my new thing. However, on the whole I'm mostly just happy there is a test suite! :smile:

Thank you for the review and the improvement suggestion. I've implemented it in the update I've just pushed.

econchick commented 2 years ago

uff - @ErwinJunge , looks like that small change broke some tests 😬 If you don't have time to fix them in the next couple of days, I can try to dig in. (This indeed makes me want to rewrite the test suite 🤦🏻)

ErwinJunge commented 2 years ago

Tests updated in latest push.

Re: pycon US, thanks for the offer but I don't think that'll be likely :) I live in the Netherlands, and it's currently a bit hard to travel out of Europe :sweat_smile:

ErwinJunge commented 2 years ago

@econchick those test failures look like the docstring coverage results are different on windows than they are on linux :confused: I don't have a windows box available to run those tests locally and can't imagine what would make the results different on windows so I don't think I'll be able to fix these.

ErwinJunge commented 2 years ago

@econchick do you have some input on the windows tests? Is there perhaps something I can do without having access to a windows box?

ErwinJunge commented 2 years ago

@econchick I took a closer look today and what's failing are the test expectation files in tests/functional/fixtures/windows/ I didn't update those (because I got all green on my machine :sweat_smile: ). However, I don't think I can update these, since I don't have access to a windows box to recreate them on. I tried copy-pasting the non-windows files and checking what changed using git diff, but that indicates the entire file is different (including stuff like the first line that definitely didn't change). There's some windows-specific magic going on that I can't replicate without access to a windows box. Can you regenerate those files and update the PR for me?

What I did for the linux files is this:

  1. Add the below into a relevant test
    with open(expected_fixture, "w") as f:
        f.write(expected_out)
  2. Sanity check the git diff. There might be added lines that shouldn't be there, remove those. Make sure there are only changed lines that make sense according to the python changes in the PR.
  3. Once the diff is clean: commit :)
ErwinJunge commented 2 years ago

@econchick friendly ping on the above :) Please let me know if there's anything I can do to move this along.

ErwinJunge commented 2 years ago

:wave: @econchick please don't forget about this one :pleading_face:

conrad-stork commented 10 months ago

Hi All,

I found this pull request as I the same problem with interrogate. My question would be if there is any chance that this pull request is merged at some point? I think this could be very useful feature.

Tagging here: @ErwinJunge @econchick

Thanks and Best