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

Set up system of code examples and details for message documentation #5934

Merged
merged 4 commits into from
Mar 23, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Mar 18, 2022

  • Add yourself to CONTRIBUTORS.txt if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
✨ New feature
📜 Docs

Description

This closes #5527. I'd like to create a separate issue to track the progress of migrating the code examples of pylint-errors.

This allows for each message to have a good.py, bad.py, related.rst and details.rst section. I have added two examples which can be viewed here and here. Based on this and this. Note that the original example is actually incorrect.
The bad.py is written in the same style as the functional test so that at one point we could test against them. For now, I have considered that out of scope. (I also want to find a way to not do that every PR as not to inflate test time).

I think the structure works but let me know what you think!

@coveralls
Copy link

coveralls commented Mar 18, 2022

Pull Request Test Coverage Report for Build 2008171386

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.078%

Totals Coverage Status
Change from base Build 2002970185: 0.0%
Covered Lines: 15188
Relevant Lines: 16144

💛 - Coveralls

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Great!

Comment on lines 23 to 27
PYLINT_MESSAGES_PATH = PYLINT_BASE_PATH / "doc" / "messages"
"""Path to the messages documentation folder."""

PYLINT_MESSAGES_DATA_PATH = PYLINT_BASE_PATH / "doc" / "data" / "messages"
"""Path to the messages documentation folder."""
Copy link
Member

Choose a reason for hiding this comment

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

Can we distinguish these two docstrings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, that's a mistake. Fixing!

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM ! Thank you. Glad this is getting some traction again. This is important, yet we have a lot to do for 2.13 already. I made some suggestions about redactional style for the documentation, let me know what you think.

Comment on lines +1 to +2
def foo():
"""A dummy description."""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def foo():
"""A dummy description."""
def add(a: Any, b: Any) -> Any:
"""Add a and b."""
return a + b

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean, what does adding Any really provide here? Without any information mypy and others will already infer everything as Any.

Comment on lines +1 to +2
def foo():
pass # [emtpy-docstring]
Copy link
Member

Choose a reason for hiding this comment

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

Not related to the actual MR, but I think code example could be 'real' and 'exciting' with animal, vegetal and real concepts or code example to be more rememberable. Also typing by default would influence begginner to use it later.

Suggested change
def foo():
pass # [emtpy-docstring]
def add(a: Any, b: Any) -> Any:
return a + b # [emtpy-docstring]

Foos and bars are traditional and have the benefit of not introducing more concept than the message itself, but also a little boring and I think human learn better with (fun) examples than with generic examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, in this case it doesn't really hurt, but I actually think the examples should be as bare bones as possible. That helps to identify the actual issue.
To make a (bad) example from this case: is empty-docstring raise for all functions or only for functions with parameters? In this case it doesn't really matter because that though is nonsensical, but I think there are messages where it actually helps if the example is as simple as possible.

Copy link
Member

Choose a reason for hiding this comment

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

You're right about "any", maybe we could another example ? (def do_nothing() -> None:).

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas Mar 21, 2022

Choose a reason for hiding this comment

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

I think you're also right about barebone being a good thing, but I dislike foo/bar for documentation. For a beginner the first thing they have to do is to read https://en.wikipedia.org/wiki/Foobar and learn about our little software programmer internal meme. We can probably make barebone examples that school children would understand without prior knowledge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah shit, I completely forgot about this.

I'll keep it in mind for future PRs 👍

Copy link
Member

Choose a reason for hiding this comment

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

Don't worry we'll have to review hundred of examples that were in pylint-error, there's no use nitpicking about the style right now 😄

Comment on lines +1 to +7
async def foo():
def _inner_foo():
yield from [1, 2, 3]


async def foo():
yield 42
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def foo():
def _inner_foo():
yield from [1, 2, 3]
async def foo():
yield 42
async def count_to_three() -> Iterator[int]:
def _inner_count_to_three() -> Iterator[int]:
yield from [1, 2, 3]
async def the_answer() -> Iterator[int]:
yield 42

Comment on lines +1 to +2
async def foo():
yield from [1, 2, 3] # [yield-inside-async-function]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def foo():
yield from [1, 2, 3] # [yield-inside-async-function]
async def count_to_three() -> Iterator[int]:
yield from [1, 2, 3] # [yield-inside-async-function]

@Pierre-Sassoulas Pierre-Sassoulas merged commit 536087e into pylint-dev:main Mar 23, 2022
@DanielNoord DanielNoord deleted the messages-documentation branch March 23, 2022 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate content from pylint-errors into the new pages for message
4 participants