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

Improve the contributing guidelines. #760

Merged
merged 6 commits into from
Jan 17, 2024
Merged

Improve the contributing guidelines. #760

merged 6 commits into from
Jan 17, 2024

Conversation

tristanlatr
Copy link
Contributor

@tristanlatr tristanlatr commented Jan 2, 2024

fixes #752
fixes #753
fixes #756
fixes #757

Copy link

codecov bot commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1b66928) 92.69% compared to head (abb5b87) 92.69%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #760   +/-   ##
=======================================
  Coverage   92.69%   92.69%           
=======================================
  Files          47       47           
  Lines        8285     8285           
  Branches     1826     1826           
=======================================
  Hits         7680     7680           
  Misses        347      347           
  Partials      258      258           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tristanlatr
Copy link
Contributor Author

Rendered changes can be reviewed here: https://pydoctor--760.org.readthedocs.build/en/760/contrib.html

This comment has been minimized.

@tristanlatr
Copy link
Contributor Author

@buhtz you can review this change, thanks

This comment has been minimized.

docs/source/contrib.rst Outdated Show resolved Hide resolved

This comment has been minimized.

Copy link
Contributor

@buhtz buhtz left a comment

Choose a reason for hiding this comment

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

It is definitly an improvement.
What I always like is a TOC on top every documentation file. It help me to easier understand the scope of the file.

Not your fault but "bots" always do feel impolite and noisy. These machines try to imitate a human being instead of just giving status output. The information from primer and codecov might be useful for maintainers but not for extern contributors. It just blows up the discussion with noise and drive less experienced contributors away. Codecov is useless in this PR because code was not modified. The bot should cover that.

But to close with something positive 😄 : The coverage value is impressive!

docs/source/contrib.rst Show resolved Hide resolved
- ``tox -e test``
- ``pip install '.[test]' && pytest pydoctor``
* - Run pyflakes
- ``tox -e pyflakes``
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you run a linter. You #757 you told me that you don't.

#757 is not fixed. IMHO it would be a good idea to mention your code style even if you do not care about code style. Also the latter is a "tell".

What I do not understand is why pyflakes do not throw any errors. I couldn't find a pyflakes config somewhere. But the code do not follow PEP8 and should throw 100 of errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is only about line 47-48.

Copy link
Contributor Author

@tristanlatr tristanlatr Jan 3, 2024

Choose a reason for hiding this comment

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

I meant we don't have a formatter, like black. pyflakes check for errors in the code, not the formatting.

Copy link
Contributor

@buhtz buhtz Jan 3, 2024

Choose a reason for hiding this comment

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

"errors in the code" and "formatting" in Python is often the same because the format (e.g. indention) is part of the logic.

Anyway: I mixed up "pyflakes" with "flake8". Both are linters but "pyflakes" is of the "lazy type" and by design do not check for PEP8 or similar problems. Now I understand why it runs without errors. It is kind of a "minimal linter".

What kind of "errors" would pyflakes find? It's docu do not list errors.

But even this should be an element of the contrib docu. "No explicit coding style just following the minimal rules of pyflakes" (or something like this). But don't you have rules about naming variables for example; camel case, upper case letters only, etc pp? Do you accept PRs with line length of 120 characters?
The key of such pedantic details is that some contributors won't start a PR when they have to be scared that the maintainer will reject the PR because of (unknown) style reasons. Maintainers should state very clear what they want and what not. Otherwise contributors wouldn't risk to burn their resources.

Copy link
Contributor Author

@tristanlatr tristanlatr Jan 3, 2024

Choose a reason for hiding this comment

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

Pyflakes will catch unused imports, name error and other relatively obvious mistakes. For the coding style I think we can just link the PEP8 guidelines. Pydoctor is about 20 years old, and we have vendored some code from other packages as well: so we can’t really enforce the same style everywhere. I’ll accept long line if it’s justified, it’s up to the reviewer to request refactors if some proposed change is very ugly.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice explanation and could fit well into a contrib file.

@tristanlatr
Copy link
Contributor Author

The information from primer and codecov might be useful for maintainers but not for extern contributors. It just blows up the discussion with noise and drive less experienced contributors away.

Even external contributors should be aware of the missing coverage lines or the potential new warnings triggered after a their changes. So I’de say it’s still relevant.

This comment has been minimized.

Copy link

According to pydoctor_primer, this change doesn't affect pydoctor warnings on a corpus of open source code. ✅

@tristanlatr tristanlatr merged commit 9becf85 into master Jan 17, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants