Add doc_node attribute #1276

kasium · 2021-11-26T13:53:07Z

Steps

For new features or bug fixes, add a ChangeLog entry describing what your PR does.
Write a good description on what the PR does.

Description

This change adds a new attribute doc_node to ClassDef, FunctionDef, AsyncFunctionDef and Module. This is needed, since some checks in pylint not only need the docstring as a string, but the node containing it. For details see pylint-dev/pylint#3077

Type of Changes

	Type
✓	✨ New feature

Related Issue

pylint-dev/pylint#3077

Pierre-Sassoulas

Thank you for this, I expected more work to be fair, this is rather elegant. As it's a "big" features I'll wait for opinion on it and it's going to be in astroid 2.10.

tests/unittest_nodes.py

DanielNoord · 2021-11-27T22:39:35Z

I haven't reviewed the code yet, but can't this be added to all nodes?

VARIABLE = 1
"""A variable I will use somewhere"""

IDEs know that this docstring belongs to the variable. If we're creating this new attribute we might as well try and do it for all nodes? This would allows allow us to call NodeNG.doc_node instead of having to do an isinstance first.

kasium · 2021-11-29T14:57:18Z

I haven't reviewed the code yet, but can't this be added to all nodes?
VARIABLE = 1
"""A variable I will use somewhere"""
IDEs know that this docstring belongs to the variable. If we're creating this new attribute we might as well try and do it for all nodes? This would allows allow us to call NodeNG.doc_node instead of having to do an isinstance first.

To doc_node is only used for four nodes, so does it really make sense to add it to all. Especially if doc is also just present for four nodes?

DanielNoord · 2021-11-29T17:17:04Z

To doc_node is only used for four nodes, so does it really make sense to add it to all. Especially if doc is also just present for four nodes?

As I said, one of the benefits would be that we can do node.doc_node without first inferring the type of node without running into issues. This is quite a nice benefit!
A second thing is that we never know when we might need this information. Perhaps somebody would like to make a highly opinionated checker that forces all global constants to have a docstring? We might as well implement this in its fulllest and most productive form 😄

By the way, I'm happy to assist in changing this so it is added to all node types! Just let me know if I can help!

kasium · 2021-11-29T19:00:48Z

@DanielNoord okay, the let me add it to all nodes. Which is the right class for that?

DanielNoord · 2021-11-29T19:03:47Z

@DanielNoord okay, the let me add it to all nodes. Which is the right class for that?

NodeNG is the base class used by all other nodes. That would be the first place to start looking.

kasium · 2021-11-30T14:40:43Z

@DanielNoord okay, the let me add it to all nodes. Which is the right class for that?

NodeNG is the base class used by all other nodes. That would be the first place to start looking.

done

for more information, see https://pre-commit.ci

DanielNoord

Sorry for taking a little longer to review this!

Although this wouldn't actually add doc_node to Const nodes, I think it is a good start. Adding it as an attribute to NodeNG makes sense to me anyway.

Please let me know if you have any questions! Happy to help move this along 😄

.gitignore

astroid/nodes/node_ng.py

astroid/nodes/scoped_nodes.py

tests/unittest_nodes.py

astroid/rebuilder.py

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

for more information, see https://pre-commit.ci

DanielNoord

@cdce8p Not sure if this was ready for review yet, but I thought it would be good to keep up with what you're doing so we can merge this as soon as it is done. Please dismiss if this is not ready yet.
I only have minor questions and comments though.

Could you also add a test case for return annotation in string format for a function definition? From my initial read of the code I wondered if this would be handled correctly. I tested, and it does (why would I think otherwise with one of your PR's 😅) but it would be good to have a test case as well.

astroid/rebuilder.py

ChangeLog

cdce8p · 2022-02-28T22:05:02Z

@kasium Sorry for taking over your PR. You already did an awesome job 🚀 Unfortunately, this PR blocks the next pylint release. Combined with the necessary changes, I though that it would be faster to work on it myself.

@cdce8p Not sure if this was ready for review yet, but I thought it would be good to keep up with what you're doing so we can merge this as soon as it is done. Please dismiss if this is not ready yet. I only have minor questions and comments though.

It is ready now 😄

Could you also add a test case for return annotation in string format for a function definition? From my initial read of the code I wondered if this would be handled correctly. I tested, and it does (why would I think otherwise with one of your PR's 😅) but it would be good to have a test case as well.

Done. Although string return annotations weren't an issue, I only accepted comments for Module docstrings. Noticed that this might be useful as I was writing the test case. It's fixed now.

astroid/nodes/scoped_nodes/scoped_nodes.py

DanielNoord · 2022-02-28T22:27:46Z

astroid/rebuilder.py

        data = "\n".join(self._data[lineno - 1 : end_range])

        found_start, found_end = False, False
        open_brackets = 0
-        skip_token: set[int] = {token.NEWLINE, token.INDENT}
+        skip_token: Set[int] = {token.NEWLINE, token.INDENT}


I actually saw this during review and thought I had learned something new from you (set being able to used on < 3.8/9) but alas... 😄

It can actually be used be used in 3.7 and 3.8 (with from __future__ import annotations), but not in 3.6.
Really time to remove support for 3.6 soon.

Technically, you could use "set[int]" (note the quotation marks) in 3.6, but that isn't really worth it.

Yeah, I have been looking forward to seeing you use your typing updater tool 😄

DanielNoord

I think we don't need a test for the order issue as it will be found in #1411

cdce8p · 2022-02-28T22:31:27Z

I think we don't need a test for the order issue as it will be found in #1411

Sounds good!

Have you been able to test the pylint issue? Will this PR resolve it?

DanielNoord · 2022-02-28T22:32:41Z

I think we don't need a test for the order issue as it will be found in #1411

Sounds good!

Have you been able to test the pylint issue? Will this PR resolve it?

Not tested, but I'm fairly positive it should. See my comment in #1411. If those tests pass pylint will pass as well.
I'll do a final check just to be sure. Give me a minute.

Edit:
@cdce8p Fixed 😄

cdce8p · 2022-02-28T22:40:03Z

Edit: @cdce8p Fixed 😄

I'll do one last check against Home Assistant, just to make sure and merge it afterwards. That way we can both still work on the two followups today and hopefully have the release ready tomorrow.

Around ~10min.

Updated files

DanielNoord · 2022-02-28T22:41:32Z

Edit: @cdce8p Fixed 😄

I'll do one last check against Home Assistant, just to make sure and merge it afterwards. That way we can both still work on the two followups today and hopefully have the release ready tomorrow.

Around ~10min.

@cdce8p Sounds good to me! Ping me when you need me 😄

My PR is ready. You can review it if you want. It's just a bunch of tests now.

kasium · 2022-03-01T10:59:53Z

Thanks for finishing it!

Add doc-node attribute

06c6cb3

Pierre-Sassoulas requested changes Nov 27, 2021

View reviewed changes

tests/unittest_nodes.py Outdated Show resolved Hide resolved

Pierre-Sassoulas added this to the 2.10.0 milestone Nov 27, 2021

Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Nov 27, 2021

DanielNoord self-requested a review November 27, 2021 22:40

End info

1183453

Move doc node to base

0b6c255

kasium requested a review from Pierre-Sassoulas November 30, 2021 14:41

kasium and others added 5 commits November 30, 2021 14:45

Fix issue

a437d85

Fix

ae28749

[pre-commit.ci] auto fixes from pre-commit.com hooks

f308dc9

for more information, see https://pre-commit.ci

Fix

cc59a2c

Merge branch 'issue-3077' of github.com:kasium/astroid into issue-3077

2b49307

DanielNoord requested changes Dec 1, 2021

View reviewed changes

kasium and others added 8 commits December 1, 2021 22:20

Update astroid/nodes/node_ng.py

e926ad3

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Reset .gitignore

861bf47

Add types

5bb2942

Remove types from docstring

cefbe62

[pre-commit.ci] auto fixes from pre-commit.com hooks

922575a

for more information, see https://pre-commit.ci

Add end_line/end_column

0577255

Merge branch 'issue-3077' of github.com:kasium/astroid into issue-3077

1138b00

Improve tests

5150ef0

kasium requested a review from DanielNoord December 1, 2021 21:38

Fix weird thing

a0b314c

cdce8p added 5 commits February 28, 2022 14:47

Fix doc_node position attributes

1a593ca

Fix

5e2324e

Fix 2

bc6f0e8

Fix 3

c43eb9c

Small improvements

847a2fa

DanielNoord requested changes Feb 28, 2022

View reviewed changes

astroid/rebuilder.py Outdated Show resolved Hide resolved

astroid/rebuilder.py Show resolved Hide resolved

astroid/rebuilder.py Outdated Show resolved Hide resolved

astroid/rebuilder.py Show resolved Hide resolved

Small updates and last fixes

d3f7b8b

DanielNoord approved these changes Feb 28, 2022

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

cdce8p changed the title ~~Add doc-node attribute~~ Add doc_node attribute Feb 28, 2022

Move changelog entry to 2.11

4507205

cdce8p removed the Blocked 🚧 A PR or issue blocked by another PR or issue label Feb 28, 2022

cdce8p modified the milestones: 2.12.0, 2.11.0 Feb 28, 2022

DanielNoord self-requested a review February 28, 2022 22:13

DanielNoord requested changes Feb 28, 2022

View reviewed changes

astroid/nodes/scoped_nodes/scoped_nodes.py Outdated Show resolved Hide resolved

cdce8p added 2 commits February 28, 2022 23:18

Fix astroid_fields order

c5f9181

Fix or ignore pylint issues

fe86d0d

DanielNoord reviewed Feb 28, 2022

View reviewed changes

cdce8p mentioned this pull request Feb 28, 2022

Deprecate doc attribute? #1414

Closed

DanielNoord approved these changes Feb 28, 2022

View reviewed changes

cdce8p merged commit c0d2e5c into pylint-dev:main Feb 28, 2022

kasium deleted the issue-3077 branch March 1, 2022 10:59

DanielNoord mentioned this pull request Mar 2, 2022

Disable bad-docstring-quotes for <= 3.7 pylint-dev/pylint#5526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add doc_node attribute #1276

Add doc_node attribute #1276

kasium commented Nov 26, 2021

Pierre-Sassoulas left a comment

DanielNoord commented Nov 27, 2021

kasium commented Nov 29, 2021

DanielNoord commented Nov 29, 2021

kasium commented Nov 29, 2021

DanielNoord commented Nov 29, 2021

kasium commented Nov 30, 2021

DanielNoord left a comment

DanielNoord left a comment

cdce8p commented Feb 28, 2022

DanielNoord Feb 28, 2022

cdce8p Feb 28, 2022

DanielNoord Feb 28, 2022

DanielNoord left a comment

cdce8p commented Feb 28, 2022

DanielNoord commented Feb 28, 2022 •

edited

Loading

cdce8p commented Feb 28, 2022

DanielNoord commented Feb 28, 2022

kasium commented Mar 1, 2022

Add doc_node attribute #1276

Add doc_node attribute #1276

Conversation

kasium commented Nov 26, 2021

Steps

Description

Type of Changes

Related Issue

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Nov 27, 2021

kasium commented Nov 29, 2021

DanielNoord commented Nov 29, 2021

kasium commented Nov 29, 2021

DanielNoord commented Nov 29, 2021

kasium commented Nov 30, 2021

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

cdce8p commented Feb 28, 2022

DanielNoord Feb 28, 2022

Choose a reason for hiding this comment

cdce8p Feb 28, 2022

Choose a reason for hiding this comment

DanielNoord Feb 28, 2022

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

cdce8p commented Feb 28, 2022

DanielNoord commented Feb 28, 2022 • edited Loading

cdce8p commented Feb 28, 2022

DanielNoord commented Feb 28, 2022

kasium commented Mar 1, 2022

DanielNoord commented Feb 28, 2022 •

edited

Loading