Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9461 pydoctor warnings #1021

Merged
merged 8 commits into from
Jul 9, 2018
Merged

9461 pydoctor warnings #1021

merged 8 commits into from
Jul 9, 2018

Conversation

twm
Copy link
Contributor

@twm twm commented Jun 3, 2018

Clear some pydoctor/epytext warnings I noticed while working on #1020. The pywin32 errors appear be legitimate but not fixable, as the ActiveState docs there have disappeared: http://docs.activestate.com/activepython/2.7/pywin32/PyWin32.html is a 404.

Contributor Checklist:

@twm twm requested a review from a team June 3, 2018 01:14
@adiroiban
Copy link
Member

Thanks for looking into this.
AFAIK we have a test for documentation and the build should fail if we have pydoctor warnings.
Maybe we can take this opportunity to also fix the test :)

@adiroiban
Copy link
Member

So it looks like there is no test to check that we don't introduce new warnings in future PR.
I remember that I worked on this at some time... but not sure how and why it was changed.


Or maybe it was done only for buildbot

In buildbot I see that we got errors in step 6, yet the build is green

https://buildbot.twistedmatrix.com/builders/documentation/builds/4014


Note that now we also run epydoc build on Circle-CI so maybe as part of TDD process we can look at updating tox -r -e apidocs to fail.

What do you think?

@twm
Copy link
Contributor Author

twm commented Jun 26, 2018

@adiroiban I actually wrote the ticket for this with a deliberately narrow scope. Basically, I wanted to fix the warning I introduced.

Since pydoctor warnings are issued based on external resources over which we have no control, I'm not too keen on making them break the build. I feel that it is important that we seek and destroy false positives. I mean, there hasn't been much merged here in the last month, has there? I note that Travis started reporting random failures of twisted.python.test.test_sendmsg.CModuleSendmsgTests.test_shortsend about a month ago. A broken build is discouraging to contributors.

Overall, I would prefer that the perfect not be the enemy of the good. Do these changes look okay to merge?

@codecov
Copy link

codecov bot commented Jun 29, 2018

Codecov Report

Merging #1021 into trunk will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            trunk    #1021      +/-   ##
==========================================
- Coverage   91.89%   91.83%   -0.07%     
==========================================
  Files         844      842       -2     
  Lines      150605   150339     -266     
  Branches    13142    13145       +3     
==========================================
- Hits       138397   138059     -338     
- Misses      10116    10180      +64     
- Partials     2092     2100       +8

Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be unambiguously correct. Please land at your convenience!

@@ -0,0 +1 @@
Several minor formatting problems in the API documentation have been corrected.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be more a .misc, to be honest.

@twm twm removed the needs-review label Jul 9, 2018
@twm twm merged commit 8d15764 into trunk Jul 9, 2018
@twm twm deleted the 9461-pydoctor-warnings branch July 9, 2018 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants