gh-75710: IDLE: Add docstrings and tests to some editor.py functions #3669

csabella · 2017-09-20T13:10:44Z

Add docstrings and test for editor.py functions related to the reload from configdialog, creating the menubar, and applying keybindings.

https://bugs.python.org/issue31529

Issue: IDLE: Add docstrings and tests for editor.py reload functions #75710

csabella · 2017-09-20T13:19:13Z

I was trying to understand the editor functions accessed from configdialog so that I could test those and thought the best way to understand what was really happening was to document and test the functions being called.

Some notes:

I haven't done ResetColorizer because that seemed like its own rabbithole.
I had to add a windows-only test because os.startfile is a windows only function and wouldn't work on my system. Hope it works.
I was getting the timer_event CodeContext errors when running the tests, so I added a line to _close() in editor.py to unbind that event before deleting the text instance and it made the error go away. If it's the right fix, you may want to pull that out into its own PR.

Sorry that this is still massive. I really tried to limit it to what I needed to look at.

terryjreedy · 2017-09-21T16:19:26Z

Lib/idlelib/idle_test/test_editor.py

@@ -1,14 +1,432 @@
+""" Test idlelib.editor.
+"""


I will add a coverage line so we know how far we yet have to go.

I later did, which is part of the merge conflict.

Done, part of the merge conflict.

taleinat

Amazing work adding so many tests for the editor window! I think these are sorely needed, and it's too bad they've been languishing here for a while now.

I've got a bunch of review comments, but overall this is a huge improvement, and I'd like to help get it through!

taleinat · 2019-08-09T20:32:30Z

Lib/idlelib/editor.py

+
+        This should be called before the keybindings are applied
+        in ApplyKeyBindings() otherwise the old bindings will still exist.
+        Note: this does not remove the Tk/Tcl keybindings attached to


Add another blank line before the "Note:".

Or leave out 'Note:'. I am going to revise these added docstrings.

taleinat · 2019-08-09T20:33:23Z

Lib/idlelib/editor.py

@@ -414,6 +414,21 @@ def set_line_and_column(self, event=None):


    def createmenubar(self):
+        """Populate the menu bar widget for the editor window.
+
+        Each option on the menubar is itself a cascade-type Menu widget


IMO in this case, the rest of the doc-string after the first line should be a comment instead, since it describes the method's code rather than it's purpose.

The problem goes deeper. There should be separate window-with-menu, frame, and text classes, and possibly an IDLE-menu class with a docstring that explains the menu system. And it maybe should be in mainmenu.py.

At present, the menu functions are scattered around this file. Right now, they should be at least gathered together in one section with one class-level block comment. Or maybe the added comment should be in mainmenu.py. This should be a separate PR.

taleinat · 2019-08-09T20:34:28Z

Lib/idlelib/editor.py

        # Called from configdialog.py
        self.mainmenu.default_keydefs = keydefs = idleConf.GetCurrentKeySet()
        self.apply_bindings()
        for extensionName in self.get_standard_extension_names():
            xkeydefs = idleConf.GetExtensionBindings(extensionName)
            if xkeydefs:
                self.apply_bindings(xkeydefs)
-        #update menu accelerators
+        # Update menu accelerators.
+        # XXX - split into its own function and call it from here?


Personally, I don't like adding such comments to the code.

Such comments should have initials and at least a year. But yes, they can hang around for decades. In this case I agree, as a separate method is needed to group together menu methods. Understanding this code requires knowing the menu term definitions.

taleinat · 2019-08-09T20:36:43Z

Lib/idlelib/editor.py

-        "Update the additional help entries on the Help menu"
+        """Update the additional help entries on the Help menu.
+
+        First the existing additional help entries are removed from


In this doc-string, I find the added description of how the method works entirely unnecessary, because it is already similarly detailed in comments.

taleinat · 2019-08-09T20:38:18Z

Lib/idlelib/editor.py

-        "Create a callback with the helpfile value frozen at definition time"
+        """Create a callback with the helpfile value frozen at definition time.
+
+        Args:


This style of describing args in doc-strings is non-standard. I think it would be shorter and simpler to just remove the "Args:" heading, and change the "Returns:" section into a single line of text.

Likewise for other methods.

Agreed. Cheryl did this PR after we refactored configdialog. For that, we had some exceptionally wordy docstrings to keep things clear. This docstring should start with 'Return...'

taleinat · 2019-08-09T20:40:12Z

Lib/idlelib/editor.py

@@ -999,6 +1037,8 @@ def _close(self):
        if self.color:
            self.color.close(False)
            self.color = None
+        # Allow code context to close its text.after calls.


How does unbinding the event allow the code context to do anything?

Also this is surprising to find given the title of this PR. If kept, it should be mentioned in the description and in the final commit message as an additional fix.

Agreed that revising _close, which has been somewhat problematical, is a separate issue.

taleinat · 2019-08-09T20:42:56Z

Lib/idlelib/editor.py

+        Args:
+            menudefs: Menu and submenu names, underlines (shortcuts),
+                and events which is a list of tuples of the form:
+                [(menu1, [(submenu1a, '<<virtual event>>'),


Shouldn't the menu and sub-menu names in the examples be stings?

This should not be here. See previous comment about explaining menu terms once.

taleinat · 2019-08-09T20:44:56Z

Lib/idlelib/idle_test/test_editor.py

+
+def setUpModule():
+    global root, editwin
+    requires('gui')


Isn't it overly restrictive to require a GUI for all tests here? As an extreme example, testing prepstr() certainly doesn't require a GUI.

Agreed. I might be tempted to have separate test_editor_gui and test_editor_nogui files if not for the problem of coverage measurement. In any case, refactoring could isolate non-gui code for easier testing.

taleinat · 2019-08-09T20:50:26Z

Lib/idlelib/idle_test/test_editor.py

+        eq(w.menudict['file'].entrycget(3, 'label'), 'Recent Files')
+        # No items added to helpmenu, so the length has no value.
+        eq(w.base_helpmenu_length, None)
+        w.fill_menus.assert_called_with()


If see this assertion and the next one as testing the implementation rather the behavior, so I suggest removing them.

I will check whether these should merely be deleted or replace with a behavior test.

taleinat · 2019-08-09T20:51:30Z

Lib/idlelib/idle_test/test_editor.py

+        # EditorWindow that create the menubar and submenus.
+        # The class is mocked in order to prevent the functions
+        # from being called automatically.
+        w = cls.mock_editwin = mock.Mock(editor.EditorWindow)


Do I understand correctly that this uses a mock mostly to avoid all of the setup in EditorWindow.init()? That could also be accomplished by patching that method (e.g. with mock.patch) or by creating a sub-class.

We have elsewhere used a standalone class. In this case, a subclass would inherit menu_spec. I will look at how w is used.

taleinat · 2020-09-20T18:41:01Z

Ping, @csabella?

terryjreedy

I am going to first edit the new docstrings as indicated in my reponses to Tal's comments, and make a separate PR.

terryjreedy · 2020-09-20T21:48:10Z

Lib/idlelib/editor.py

@@ -414,6 +414,21 @@ def set_line_and_column(self, event=None):


    def createmenubar(self):
+        """Populate the menu bar widget for the editor window.
+
+        Each option on the menubar is itself a cascade-type Menu widget


The problem goes deeper. There should be separate window-with-menu, frame, and text classes, and possibly an IDLE-menu class with a docstring that explains the menu system. And it maybe should be in mainmenu.py.

At present, the menu functions are scattered around this file. Right now, they should be at least gathered together in one section with one class-level block comment. Or maybe the added comment should be in mainmenu.py. This should be a separate PR.

terryjreedy · 2020-09-20T21:48:18Z

Lib/idlelib/editor.py

+
+        This should be called before the keybindings are applied
+        in ApplyKeyBindings() otherwise the old bindings will still exist.
+        Note: this does not remove the Tk/Tcl keybindings attached to


Or leave out 'Note:'. I am going to revise these added docstrings.

terryjreedy · 2020-09-20T22:10:35Z

Lib/idlelib/editor.py

        # Called from configdialog.py
        self.mainmenu.default_keydefs = keydefs = idleConf.GetCurrentKeySet()
        self.apply_bindings()
        for extensionName in self.get_standard_extension_names():
            xkeydefs = idleConf.GetExtensionBindings(extensionName)
            if xkeydefs:
                self.apply_bindings(xkeydefs)
-        #update menu accelerators
+        # Update menu accelerators.
+        # XXX - split into its own function and call it from here?


Such comments should have initials and at least a year. But yes, they can hang around for decades. In this case I agree, as a separate method is needed to group together menu methods. Understanding this code requires knowing the menu term definitions.

terryjreedy · 2020-09-20T22:10:54Z

Lib/idlelib/editor.py

-        "Update the additional help entries on the Help menu"
+        """Update the additional help entries on the Help menu.
+
+        First the existing additional help entries are removed from


terryjreedy · 2020-09-20T22:13:55Z

Lib/idlelib/editor.py

-        "Create a callback with the helpfile value frozen at definition time"
+        """Create a callback with the helpfile value frozen at definition time.
+
+        Args:


Agreed. Cheryl did this PR after we refactored configdialog. For that, we had some exceptionally wordy docstrings to keep things clear. This docstring should start with 'Return...'

terryjreedy · 2020-09-20T22:27:00Z

Lib/idlelib/editor.py

    s = re.sub(r"\b\w+\b", lambda m: keynames.get(m.group(), m.group()), s)
+    # Remove Key- from string.


The first 2 added comments are helpful, the rest are obvious.

terryjreedy · 2020-09-20T22:27:46Z

Lib/idlelib/idle_test/test_editor.py

@@ -1,14 +1,432 @@
+""" Test idlelib.editor.
+"""


Done, part of the merge conflict.

terryjreedy · 2020-09-20T22:30:38Z

Lib/idlelib/idle_test/test_editor.py

+
+def setUpModule():
+    global root, editwin
+    requires('gui')


Agreed. I might be tempted to have separate test_editor_gui and test_editor_nogui files if not for the problem of coverage measurement. In any case, refactoring could isolate non-gui code for easier testing.

terryjreedy · 2020-09-20T23:46:41Z

Lib/idlelib/idle_test/test_editor.py

+        # EditorWindow that create the menubar and submenus.
+        # The class is mocked in order to prevent the functions
+        # from being called automatically.
+        w = cls.mock_editwin = mock.Mock(editor.EditorWindow)


We have elsewhere used a standalone class. In this case, a subclass would inherit menu_spec. I will look at how w is used.

terryjreedy · 2020-09-20T23:48:30Z

Lib/idlelib/idle_test/test_editor.py

+        eq(w.menudict['file'].entrycget(3, 'label'), 'Recent Files')
+        # No items added to helpmenu, so the length has no value.
+        eq(w.base_helpmenu_length, None)
+        w.fill_menus.assert_called_with()


I will check whether these should merely be deleted or replace with a behavior test.

bedevere-bot · 2020-09-21T00:02:01Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

terryjreedy · 2020-09-21T00:05:17Z

Whoops, by responding to Tal's comments as part of a new review, my responses got listed twice, once under Tal's comment where they below and then again separately, without the context.

terryjreedy · 2020-09-21T00:43:44Z

I minimally resolved the merge conflict in the web box so I could proceed. This did a full merge update rather than just updating the one file. There is still the redundancy of global requires gui and class requires gui to be fixed, and Tal's other conflicts. Since I had to do this, I will go ahead and edit everything on this PR.

More tk. prefixes.

terryjreedy · 2020-09-21T02:06:21Z

I stopped the Windows test failures by commenting out the failing lines with an explanation.

The Ubuntu failure is

  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/idlelib/idle_test/test_editor.py", line 422, in test__extra_help_callback_not_windows
    ehc = partial(w._EditorWindow__extra_help_callback, w)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
AttributeError: Mock object has no attribute '_EditorWindow__extra_help_callback'. Did you mean: '_EditorWindow__recent_file_callback'?

May do the same here.

terryjreedy · 2020-09-21T06:34:38Z

I think editor.py is more or less commit ready. Tal, review again or not as you wish.

Test_editor is not ready. test_editor passes, but still needs requires('gui') restricted. I want to try Tal's suggested alternative. And also make sure that every test is actually testing. The test I revised was not, as the function supposedly being tested was mocked.

test_mainmenu passes by itself, but test_idle fails with

File "f:\dev\3x\lib\idlelib\idle_test\test_mainmenu.py", line 17, in test_default_keydefs
self.assertGreaterEqual(len(mainmenu.default_keydefs), 50)
AssertionError: 5 not greater than or equal to 50

5 happens to be the length of keydefs used in test_editor, which run first, so the test change is somehow affecting mainmenu. This is probably via changing something in config. test_editor does not import config, but editor does.

terryjreedy · 2021-02-20T06:13:12Z

Additional test failure: Windows error box titled "Document Start Failure" with message "(X) boom". Test freezes until box is dismissed. I suspect a test of additional help resources needs mocks for both Windows and *nix.

Commit extracted from PR python#3669. Will edits more later. Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

Commit extracted from PR #3669. Will edit more later. Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

…ythonGH-104446) Commit extracted from PR pythonGH-3669. Will edit more later. (cherry picked from commit 46f1c78) Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

…H-104446) (#104450) gh-75710: IDLE - add docstrings and comments to editor module (GH-104446) Commit extracted from PR GH-3669. Will edit more later. (cherry picked from commit 46f1c78) Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu> Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

github-actions · 2023-06-14T00:10:30Z

This PR is stale because it has been open for 30 days with no activity.

csabella added 2 commits September 20, 2017 08:57

IDLE: Add docstrings and tests for editor.py

c21d679

Add blurb

d047c39

the-knights-who-say-ni added the CLA signed label Sep 20, 2017

bedevere-bot added the awaiting review label Sep 20, 2017

terryjreedy reviewed Sep 21, 2017

View reviewed changes

csabella mentioned this pull request Jan 20, 2019

gh-79951: IDLE - Convert menudefs to dictionary #11615

Open

taleinat reviewed Aug 9, 2019

View reviewed changes

terryjreedy requested changes Sep 21, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 21, 2020

Merge branch 'master' into binding_tests

435ea82

terryjreedy added 3 commits September 20, 2020 21:13

Synchronize tkinter import in test_editor

ac8dae4

Update test_editor.py

eb25140

More tk. prefixes.

Remove test of removed _filename_to_unicode method.

8dfbd49

terryjreedy added 3 commits September 20, 2020 23:44

Remove unneeded extra underscore.

0308f80

Make test pass; delete redundant asserts.

c32379b

Docstring and comment and a couple code revisions.

ba274a4

terryjreedy added 2 commits January 25, 2021 07:37

Merge branch 'master' into binding_tests

b5d80cb

Merge remote-tracking branch 'upstream/master' into pr_3669

3fbf6da

github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 23, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot removed the stale Stale PR or inactive for long period of time. label Jul 29, 2022

github-actions bot added the stale Stale PR or inactive for long period of time. label Sep 1, 2022

hauntsaninja changed the title ~~bpo-31529: IDLE: Add docstrings and tests to some editor.py functions~~ gh-75710: IDLE: Add docstrings and tests to some editor.py functions Jan 7, 2023

github-actions bot removed the stale Stale PR or inactive for long period of time. label Jan 8, 2023

github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 7, 2023

Merge remote-tracking branch 'upstream/main' into pr_3669

0aea634

python deleted a comment from github-actions bot May 13, 2023

whitespace

cf8044e

terryjreedy added a commit to terryjreedy/cpython that referenced this pull request May 13, 2023

pythongh-75710: IDLE - add docstrings and comments to editor module

4d47692

Commit extracted from PR python#3669. Will edits more later. Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

terryjreedy mentioned this pull request May 13, 2023

gh-75710: IDLE - add docstrings and comments to editor module #104446

Merged

csabella mentioned this pull request Jul 27, 2022

IDLE: Add docstrings and tests for editor.py reload functions #75710

Open

Merge branch 'main' into binding_tests

fba2ef2

terryjreedy added a commit that referenced this pull request May 13, 2023

gh-75710: IDLE - add docstrings and comments to editor module (#104446)

46f1c78

Commit extracted from PR #3669. Will edit more later. Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

miss-islington mentioned this pull request May 13, 2023

[3.11] gh-75710: IDLE - add docstrings and comments to editor module (GH-104446) #104450

Merged

Merge branch 'main' into binding_tests

47ebc2d

github-actions bot removed the stale Stale PR or inactive for long period of time. label May 14, 2023

github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 14, 2023

		s = re.sub(r"\b\w+\b", lambda m: keynames.get(m.group(), m.group()), s)
		# Remove Key- from string.

gh-75710: IDLE: Add docstrings and tests to some editor.py functions #3669

Are you sure you want to change the base?

gh-75710: IDLE: Add docstrings and tests to some editor.py functions #3669

Conversation

csabella commented Sep 20, 2017 • edited by bedevere-bot Loading

csabella commented Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleinat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleinat commented Sep 20, 2020

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 21, 2020

terryjreedy commented Sep 21, 2020

terryjreedy commented Sep 21, 2020

terryjreedy commented Sep 21, 2020 • edited Loading

terryjreedy commented Sep 21, 2020

terryjreedy commented Feb 20, 2021

github-actions bot commented Jun 14, 2023

csabella commented Sep 20, 2017 •

edited by bedevere-bot

Loading

csabella commented Sep 20, 2017 •

edited

Loading

terryjreedy commented Sep 21, 2020 •

edited

Loading