bpo-38307:completes the Stack implementation to yield ending line for each class. #16466

kebab-mai-haddi · 2019-09-28T17:27:59Z

38307: Provide class' end line as well in order to provide the scope of the class in a module.

https://bugs.python.org/issue38307

the-knights-who-say-ni · 2019-09-28T17:28:02Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@avisrivastava254084

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

kebab-mai-haddi · 2019-09-28T17:39:18Z

@the-knights-who-say-ni I have done the CLA. When will it get updated?

brandtbucher · 2019-09-29T16:49:09Z

Thanks @avisrivastava254084, and welcome to CPython!

Regarding the CLA, the link the bot provided says the following:

After signing the CLA, please wait at least one US business day and then check the status by going to the check-python-cla website. The check will also be run automatically the next time you push changes to your PR.

It's my understanding that these signatures are processed by a human at the PSF, so it probably won't happen on a Saturday night / Sunday morning!

Do you mind updating the PR title to with "bpo-" in front of the issue number, so this can be linked appropriately?

brandtbucher

@avisrivastava254084: this is a very noisy patch. The real changes only affect about 8 lines, but you've changed over 30. Please revert all of the unrelated formatting/whitespace changes (as well as a few others) that I've highlighted in the review below.

Do you mind explaining why this is necessary? In particular, I'm concerned about:

...why we need endline. Do you have a specific use case?
...if frames[1].frame.f_locals['start'][0] - 1 is the correct way to calculate this.
...if doing it in __delitem__ of a custom list subclass is the right way to do this (I'm almost certain it's not).

At a minimum, this will need tests and a NEWS entry.

brandtbucher · 2019-09-29T16:49:49Z

Lib/pyclbr.py

@@ -51,6 +51,7 @@

 class _Object:
    "Information about Python class or function."
+


Please remove all these whitespace lines that you've added.

brandtbucher · 2019-09-29T16:50:56Z

Lib/pyclbr.py

@@ -80,6 +83,20 @@ def _addmethod(self, name, lineno):
        self.methods[name] = lineno


+class Stack(list):
+
+    def __init__(self):


This __init__ method isn't needed, since all it does is import inspect into the function's scope.

brandtbucher · 2019-09-29T16:52:23Z

Lib/pyclbr.py

@@ -161,7 +182,8 @@ def _readmodule(module, path, inpackage=None):
        search_path = path + sys.path
    spec = importlib.util._find_spec_from_path(fullmodule, search_path)
    if spec is None:
-        raise ModuleNotFoundError(f"no module named {fullmodule!r}", name=fullmodule)
+        raise ModuleNotFoundError(


Please don't reformat this line. It's fine as-is!

brandtbucher · 2019-09-29T16:53:38Z

Lib/pyclbr.py

@@ -227,14 +249,14 @@ def _create_tree(fullmodule, path, fname, source, tree, inpackage):
                    del stack[-1]
                tokentype, class_name, start = next(g)[0:3]
                if tokentype != NAME:
-                    continue # Skip class with syntax error.
+                    continue  # Skip class with syntax error.


Please revert this whitespace change as well. Below also...

brandtbucher · 2019-09-29T16:53:52Z

Lib/pyclbr.py

@@ -271,7 +293,7 @@ def _create_tree(fullmodule, path, fname, source, tree, inpackage):
                if stack:
                    cur_obj = stack[-1][0]
                    cur_class = _nest_class(
-                            cur_obj, class_name, lineno, inherit)


Please revert this.

brandtbucher · 2019-09-29T16:54:53Z

Lib/pyclbr.py

@@ -377,7 +399,7 @@ def _main():
    else:
        path = []
    tree = readmodule_ex(mod, path)
-    lineno_key = lambda a: getattr(a, 'lineno', 0)


Unidiomatic, sure. But fine as-is, and unrelated to the change. Please revert.

brandtbucher · 2019-09-29T16:55:56Z

Lib/pyclbr.py

@@ -193,8 +215,8 @@ def _create_tree(fullmodule, path, fname, source, tree, inpackage):
    """
    f = io.StringIO(source)

-    stack = [] # Initialize stack of (class, indent) pairs.
-
+    # stack = []  # Initialize stack of (class, indent) pairs.


Please remove this rather than just commenting it out. It makes for a better diff.

Suggested change

# stack = [] # Initialize stack of (class, indent) pairs.

brandtbucher · 2019-09-29T16:56:44Z

Lib/pyclbr.py

-    stack = [] # Initialize stack of (class, indent) pairs.
-
+    # stack = []  # Initialize stack of (class, indent) pairs.
+    stack = Stack()  # changing the source code so as to get the ending line


This comment belongs in a commit message, not in the source code. The old comment is fine.

Suggested change

stack = Stack() # changing the source code so as to get the ending line

stack = Stack() # Initialize stack of (class, indent) pairs.

kebab-mai-haddi · 2019-10-06T19:26:44Z

@brandtbucher
Hi! Sorry for a noisy patch, had to work asap.

.why we need endline. Do you have a specific use case?
So that the class' scope is known. Right now, class' start line is known but not the end line. If we have the end line, we can know the modules imported in b/w start and end. So, we can then deduce that class A used modules: B and C. In this way, we can say that A is dependent on B and C.

This is useful in drawing and generating UML diagrams, especially in knowing which class is dependent on which classes.

I"ll make the rest of the changes as suggested.

kebab-mai-haddi · 2019-10-06T19:41:22Z

...if frames[1].frame.f_locals['start'][0] - 1 is the correct way to calculate this.

I'd want your suggestions or some guiding material that you think I should go through.

...if doing it in delitem of a custom list subclass is the right way to do this (I'm almost certain it's not).

Same as above.

@brandtbucher

… and formatting

brandtbucher · 2019-10-08T19:24:03Z

Rather than subclassing list and overriding __delitem__, just doing stack[-1].endline = start - 1 before each del stack[-1] should suffice. Also, in keeping consistency with other places in the stdlib where this info is used, I would lean towards the spelling end_lineno.

A couple of other things: if this is accepted, it will need tests, updated docs, and a NEWS entry. All of the classes and comments in this file should also reference the new attribute. And please change the title of this PR to start with "bpo-38307:" so that the bot (and humans) can find your issue!

the-knights-who-say-ni · 2020-02-04T01:37:06Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@kebab-mai-haddi

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

…nstead to the stack

kebab-mai-haddi · 2020-02-04T01:56:33Z

@brandtbucher
Apologies for the delay. I have made the changes at the code level as you suggested. However, I am unable to figure out how should I set the attribute end_lineno?

I was wondering if you could also help me with the "CLA Not Signed" issue?
I changed my username from avisrivastava254084 to kebab-mai-haddi.

…sing subclass

kebab-mai-haddi · 2020-02-08T02:06:23Z

@brandtbucher
I have made the changes. Is the current implementation alright? If so, can you guide me to the testing phase?

codecov · 2020-02-08T02:32:20Z

Codecov Report

Merging #16466 into master will decrease coverage by 3.45%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #16466       +/-   ##
===========================================
- Coverage   82.92%   79.46%    -3.46%     
===========================================
  Files        1864      384     -1480     
  Lines      569838   169290   -400548     
  Branches    42011        0    -42011     
===========================================
- Hits       472515   134530   -337985     
+ Misses      88124    34760    -53364     
+ Partials     9199        0     -9199

Impacted Files	Coverage Δ
Modules/fcntlmodule.c	`71.17% <0.00%> (-10.26%)`	⬇️
Modules/_ctypes/_ctypes_test.c	`62.67% <0.00%> (-7.90%)`	⬇️
Modules/_contextvarsmodule.c	`54.54% <0.00%> (-8.62%)`	⬇️
Modules/expat/xmlrole.c	`48.89% <0.00%> (ø)`	⬆️
Modules/clinic/mathmodule.c.h	`85.65% <0.00%> (-2.39%)`	⬇️
Modules/clinic/fcntlmodule.c.h	`60.91% <0.00%> (-2.30%)`	⬇️
Objects/codeobject.c	`82.59% <0.00%> (-2.20%)`	⬇️
Modules/syslogmodule.c	`81.25% <0.00%> (-2.09%)`	⬇️
Modules/_sqlite/util.c	`64.86% <0.00%> (-1.81%)`	⬇️
Modules/readline.c	`73.51% <0.00%> (-1.43%)`	⬇️
... and 1577 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5a7e0c...94edb5b. Read the comment docs.

brandtbucher

Again, this still needs tests (in Lib/test/test_pyclbr.py) and a NEWS entry. The documentation and docstrings will also need to be updated (basically, anywhere the current lineno attribute is referenced).

Regarding the CLA, I think you will need to link your new GitHub username to your BPO account and re-sign the CLA. Not sure, though.

Also, just so you don't get discouraged: it's great that you're taking the time to contribute to CPython, but this module has no current experts or maintainers. I am not able to merge this PR once it's ready, and there is a fair chance that no active core developer would feel comfortable adding a new feature like this - especially without significant demand.

brandtbucher · 2020-02-10T18:06:21Z

Lib/pyclbr.py

@@ -202,11 +202,13 @@ def _create_tree(fullmodule, path, fname, source, tree, inpackage):
                lineno, thisindent = start
                # Close previous nested classes and defs.
                while stack and stack[-1][1] >= thisindent:
+                    setattr(stack[-1][0], 'end_lineno', start[0] - 1)


Why did you switch to using setattr here? This should be fine, right?

Suggested change

setattr(stack[-1][0], 'end_lineno', start[0] - 1)

stack[-1][0].end_lineno = start[0] - 1

Also on the two other lines, below...

I have to set the attribute, right? Otherwise, it results into the error: <> has no attribute end_lineno.

Also, just so you don't get discouraged: it's great that you're taking the time to contribute to CPython, but this module has no current experts or maintainers. I am not able to merge this PR once it's ready, and there is a fair chance that no active core developer would feel comfortable adding a new feature like this - especially without significant demand.

I received some positive responses on Stackoverflow for this. Don't you think there should be a starting and an end line for this?

Even if you think and abide by to go with the popular demand, should I opt for a survey, etc? Is there some platform where I can ask for votes: "Should there be end_lineno?"

Thank you

I have to set the attribute, right? Otherwise, it results into the error: <> has no attribute end_lineno.

They should both be equivalent. You're certain that applying my suggested change results in an error?

Yes. My bad, apologies. I tried the direct approach to stack[-1] instead of stack[-1][0].

csabella · 2020-02-11T12:06:15Z

@kebab-mai-haddi, on the bug tracker, you should be able to edit your GitHub profile name. Select 'Your details' on the left and it should take you to a screen where you can update your info.

brandtbucher · 2020-02-11T20:00:57Z

I received some positive responses on Stackoverflow for this. Don't you think there should be a starting and an end line for this?

Even if you think and abide by to go with the popular demand, should I opt for a survey, etc? Is there some platform where I can ask for votes: "Should there be end_lineno?"

Sorry, I should have been clearer. I myself am not really opposed to adding this feature; I just don't have the necessary permissions to actually merge this PR, and I've never even seen this module before. Once I deem that it's ready, my next step is usually to request the review of a maintainer or owner of this module who does have the ability to merge it. It doesn't look like we have anybody like that, though.

If you wanted to get somebody's attention, responding to Steven's questions on the BPO issue would be a great place to start (actually, it should probably happen regardless). If that fizzles out, you could also make a post on the Python-Ideas mailing list outlining your use case, and gathering feedback. I would also link this PR and the open BPO issue there, as well.

…va254084/cpython into endline-in-readmodule-module

kebab-mai-haddi · 2020-03-24T00:51:48Z

@brandtbucher
I have added the NEWS entry. Regarding tests, I tried the following:

t1 = type(o1), o1.name, o1.file, o1.module, o1.lineno, o1.end_lineno
t2 = type(o2), o2.name, o2.file, o2.module, o2.lineno, o2.end_lineno

the actual code of which is available in the diff of this branch.

The test is failing as the children being compared are of different types. The error is:

======================================================================
ERROR: test_nested (test.test_pyclbr.PyclbrTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/aviralsrivastava/dev/cpython/Lib/test/test_pyclbr.py", line 219, in test_nested
    compare(None, actual, None, expected)
  File "/Users/aviralsrivastava/dev/cpython/Lib/test/test_pyclbr.py", line 212, in compare
    t2 = type(o2), o2.name, o2.file, o2.module, o2.lineno, o2.end_lineno
AttributeError: 'Function' object has no attribute 'end_lineno'

----------------------------------------------------------------------

Ran 6 tests in 5.524s
Merge branch 'endline-in-readmodule-module' of github.com:avisrivastava254084/cpython into endline-in-readmodule-module

FAILED (errors=1)
test test_pyclbr failed
1 test failed again:
    test_pyclbr

== Tests result: FAILURE then FAILURE ==

1 test failed:
    test_pyclbr

1 re-run test:
    test_pyclbr

Total duration: 11 sec 993 ms
Tests result: FAILURE then FAILURE
make: *** [test] Error 2

Now, I am failing to understand what and how should I write the test for the new change of end_lineno?
Any direction/lead would be greatly appreciated.

brandtbucher · 2020-03-24T18:06:24Z

@kebab-mai-haddi I'm not too familiar with this code, but a cursory scan tells me this:

pyclbr creates two types of objects from modules:

a Class for every class
a Function for every def

I believe your patch only adds the new end_lineno attribute to Class. Likely, you also want to add it to Function as well (for symmetry and completeness).

kebab-mai-haddi · 2020-03-26T03:01:16Z

@kebab-mai-haddi I'm not too familiar with this code, but a cursory scan tells me this:

pyclbr creates two types of objects from modules:

a Class for every class

a Function for every def

I believe your patch only adds the new end_lineno attribute to Class. Likely, you also want to add it to Function as well (for symmetry and completeness).

I have done that. If you check here, you will see that if the object is of type Function, I am adding end_lineno.

Thank you in advance!

compeltes the Stack implementation to yield ending line for each class.

1f25ed4

the-knights-who-say-ni added the CLA not signed label Sep 28, 2019

bedevere-bot added the awaiting review label Sep 28, 2019

kebab-mai-haddi changed the title ~~compeltes the Stack implementation to yield ending line for each class.~~ #38307: compeltes the Stack implementation to yield ending line for each class. Sep 28, 2019

brandtbucher requested changes Sep 29, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 29, 2019

brandtbucher added the type-feature A feature request or enhancement label Sep 29, 2019

kebab-mai-haddi changed the title ~~#38307: compeltes the Stack implementation to yield ending line for each class.~~ #38307: bpo-completes the Stack implementation to yield ending line for each class. Oct 6, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 6, 2019

Aviral Srivastava added 2 commits October 6, 2019 15:42

reverts whitespace and unwanted changes including functions, comments…

7d2354a

… and formatting

removes leftover whitespace in b/w functions: unwanted

c8a003d

kebab-mai-haddi changed the title ~~#38307: bpo-completes the Stack implementation to yield ending line for each class.~~ bpo-38307:completes the Stack implementation to yield ending line for each class. Oct 8, 2019

Initialize stack of (class, indent) pairs.

7e6f079

the-knights-who-say-ni removed the CLA signed label Feb 4, 2020

the-knights-who-say-ni added the CLA not signed label Feb 4, 2020

removes Stack() custom class and adds direct attribute (end_lineno) i…

b7c1ce7

…nstead to the stack

Aviral Srivastava added 2 commits February 7, 2020 21:01

corrects the attribute name to end_lineno and sets the same without u…

3466f63

…sing subclass

removes unwanted module: inspect

f2a58c6

csabella requested a review from brandtbucher February 9, 2020 18:41

brandtbucher requested changes Feb 10, 2020

View reviewed changes

csabella removed the CLA not signed label Feb 11, 2020

the-knights-who-say-ni added the CLA not signed label Feb 11, 2020

corrects the way of setting attribute

94edb5b

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 16, 2020

blurb-it bot and others added 3 commits March 16, 2020 03:03

📜🤖 Added by blurb_it.

454d305

wip: added end_lineno in tests

dc0166f

Merge branch 'endline-in-readmodule-module' of github.com:avisrivasta…

5a26a5c

…va254084/cpython into endline-in-readmodule-module

kebab-mai-haddi mentioned this pull request Apr 27, 2020

Market Launch kebab-mai-haddi/gruml#24

Open

wip

ea2090b

kebab-mai-haddi closed this Jan 25, 2021

		@@ -51,6 +51,7 @@

		class _Object:
		"Information about Python class or function."

	stack = Stack() # changing the source code so as to get the ending line
	stack = Stack() # Initialize stack of (class, indent) pairs.

	setattr(stack[-1][0], 'end_lineno', start[0] - 1)
	stack[-1][0].end_lineno = start[0] - 1

Uh oh!

bpo-38307:completes the Stack implementation to yield ending line for each class. #16466

bpo-38307:completes the Stack implementation to yield ending line for each class. #16466

Uh oh!

Conversation

kebab-mai-haddi commented Sep 28, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Sep 28, 2019

CLA Missing

Uh oh!

kebab-mai-haddi commented Sep 28, 2019

Uh oh!

brandtbucher commented Sep 29, 2019

Uh oh!

brandtbucher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kebab-mai-haddi commented Oct 6, 2019

Uh oh!

kebab-mai-haddi commented Oct 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandtbucher commented Oct 8, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 4, 2020

Recognized GitHub username

Uh oh!

kebab-mai-haddi commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kebab-mai-haddi commented Feb 8, 2020

Uh oh!

codecov bot commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csabella commented Feb 11, 2020

Uh oh!

brandtbucher commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kebab-mai-haddi commented Mar 24, 2020

Uh oh!

brandtbucher commented Mar 24, 2020

Uh oh!

kebab-mai-haddi commented Mar 26, 2020

Uh oh!

Uh oh!

kebab-mai-haddi commented Sep 28, 2019 •

edited by bedevere-bot

Loading

brandtbucher left a comment •

edited

Loading

kebab-mai-haddi commented Oct 6, 2019 •

edited

Loading

brandtbucher commented Oct 8, 2019 •

edited by bedevere-bot

Loading

kebab-mai-haddi commented Feb 4, 2020 •

edited

Loading

codecov bot commented Feb 8, 2020 •

edited

Loading

brandtbucher commented Feb 11, 2020 •

edited

Loading