bpo-30674: IDLE: add docstrings to grep.py by csabella · Pull Request #2213 · python/cpython

csabella · 2017-06-15T12:29:24Z

No description provided.

mlouielu

Thanks for the PR, some small point should be changed.

mlouielu · 2017-06-15T12:51:38Z

Lib/idlelib/grep.py


+
 class GrepDialog(SearchDialogBase):
+    "Search Dialog for Find in Files."


"""Search dialog for find in files."""

The menu item is 'Find in Files', but I agree it is not obvious and awkward if one does not notice. "Find in Files" is already in file docstring. How about "Dialog for searching multiple files."?

mlouielu · 2017-06-15T12:52:01Z

Lib/idlelib/grep.py

    needwrapbutton = 0

    def __init__(self, root, engine, flist):
+        """Create Search Dialog for searching for a phrase in the file system.


"""Create search dialog for ...

mlouielu · 2017-06-15T12:52:50Z

Lib/idlelib/grep.py

        self.recvar = BooleanVar(root)

    def open(self, text, searchphrase, io=None):
+        "Make dialog visible on top of others and ready to use."


"""Make dialog visible .... """

No. Single double quote is allowed and preferred by me. Common in stdlib.

mlouielu · 2017-06-15T12:53:10Z

Lib/idlelib/grep.py

        self.globvar.set(os.path.join(dir, "*" + tail))

    def create_entries(self):
+        "Create base entry widgets and add widget for search path."


"""Create base entry .... path."""

mlouielu · 2017-06-15T12:53:28Z

Lib/idlelib/grep.py

        self.globent = self.make_entry("In files:", self.globvar)[0]

    def create_other_buttons(self):
+        "Add check button to recurse down subdirectories."


"""Add check button ....subdirectories."""

mlouielu · 2017-06-15T12:53:44Z

Lib/idlelib/grep.py

        btn.pack(side="top", fill="both")

    def create_command_buttons(self):
+        "Create base command buttons and add button for search."


"""Create base command .... for search."""

mlouielu · 2017-06-15T12:54:49Z

Lib/idlelib/grep.py

        prog = self.engine.getprog()
        if not prog:
-            return
+            return None


return will return None. So you don't need to explicit return None

"Either all return statements in a function should return an expression, or none of them should." So if any must (return an expression), all must, but if all return None, then none must, so None can be omitted but must be omitted everywhere.

mlouielu · 2017-06-15T12:55:21Z

Lib/idlelib/grep.py

-        if self.top:
-            self.top.grab_release()
-            self.top.withdraw()
+        "Close the dialog."


"""Close the dialog."""

mlouielu · 2017-06-15T12:57:54Z

I also create a picture for the import relationship for IDLE. you can saw it here:

csabella · 2017-06-15T17:31:47Z

Terry had changed my """ quotes on a previous PR to just " when the docstring was only one line. I was following his convention with these changes.

terryjreedy · 2017-06-16T05:51:12Z

Lib/idlelib/grep.py


+
 class GrepDialog(SearchDialogBase):
+    "Search Dialog for Find in Files."


The menu item is 'Find in Files', but I agree it is not obvious and awkward if one does not notice. "Find in Files" is already in file docstring. How about "Dialog for searching multiple files."?

terryjreedy · 2017-06-16T06:02:12Z

Lib/idlelib/grep.py

@@ -1,3 +1,8 @@
+"""Grep dialog for Find in File functionality.


'Files', plural

terryjreedy · 2017-06-16T06:11:12Z

Lib/idlelib/grep.py

+    Args:
+        text: Text widget that contains the selected text for
+              default search phrase.
+        io: IOBinding with default path to search.


'iomenu.IOBinding instance'

terryjreedy · 2017-06-16T06:13:05Z

Lib/idlelib/grep.py

    needwrapbutton = 0

    def __init__(self, root, engine, flist):
+        """Create Search Dialog for searching for a phrase in the file system.


terryjreedy · 2017-06-16T06:15:42Z

Lib/idlelib/grep.py

        self.recvar = BooleanVar(root)

    def open(self, text, searchphrase, io=None):
+        "Make dialog visible on top of others and ready to use."


No. Single double quote is allowed and preferred by me. Common in stdlib.

terryjreedy · 2017-06-16T06:44:00Z

Lib/idlelib/grep.py

-                  "(Hint: right-click to open locations.)"
-                  % hits) if hits else "No hits.")
+                   "(Hint: right-click to open locations.)"
+                   % hits) if hits else "No hits.")


I believe this was technically correct as it was, but it looks better with the extra space. But I don't really like it either way (and I wrote it in this form). In 3.6+, we can do

print(f"Hits found: {hits}\n" f"(Hint: right-click to open locations.)" if hits else "No hits.") # or with 79 char lines print(f"Hits found: {hits}\n(Hint: right-click to open locations.)" if hits else "No hits.")

I like both of these better, so choose one.

Changed. I also changed the other string formatting lines to use f strings.

terryjreedy · 2017-06-16T07:00:07Z

Lib/idlelib/grep.py

-            self.top.grab_release()
-            self.top.withdraw()
+        "Close the dialog."
+        SearchDialogBase.close(self, event)


Good catch. Maybe these were once different. Now, just delete GrepDialog.close and inherit SearchDialogBase.close.

terryjreedy · 2017-06-16T07:02:39Z

Lib/idlelib/grep.py

        self.globvar.set(os.path.join(dir, "*" + tail))

    def create_entries(self):
+        "Create base entry widgets and add widget for search path."


terryjreedy · 2017-06-16T07:02:53Z

Lib/idlelib/grep.py

        self.globent = self.make_entry("In files:", self.globvar)[0]

    def create_other_buttons(self):
+        "Add check button to recurse down subdirectories."


terryjreedy · 2017-06-16T07:04:17Z

Lib/idlelib/grep.py

            self.grep_it(prog, path)
        finally:
            sys.stdout = save
+        return None


" If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None " So if no return statement return an expression, no explicit return is needed at the end. In other words, this function was within the rules and should be left as it was.

Got it. Thanks.

csabella · 2017-06-16T11:45:10Z

@mlouielu @terryjreedy Thanks for the reviews. I added an additional note to default_command to mention that the search dialog is closed when the search begins.

One other question -- when I run this, the dialog box always opens way on the bottom of my screen. Has anyone requested a change to this before? Or would it be worth mentioning on idle-dev?

mlouielu · 2017-06-16T12:40:17Z

@csabella I think your question should also post it on b.p.o. Since here is to discuss about code review. I didn't get this on macOS, the dialog show off at the left top side of the screen.

Patch by Cheryl Sabella (cherry picked from commit 65474b9)

bpo-30674: IDLE: add docstrings to grep.py

38d8e8c

the-knights-who-say-ni added the CLA signed label Jun 15, 2017

mlouielu suggested changes Jun 15, 2017

View reviewed changes

terryjreedy requested changes Jun 16, 2017

View reviewed changes

IDLE: add docstrings to grep.py

1580835

add news item

9668ba2

terryjreedy merged commit 65474b9 into python:master Jun 27, 2017

terryjreedy pushed a commit to terryjreedy/cpython that referenced this pull request Jun 27, 2017

[3.6] bpo-30674: IDLE: add docstrings to grep.py (pythonGH-2213)

3b04a3e

Patch by Cheryl Sabella (cherry picked from commit 65474b9)

terryjreedy added a commit that referenced this pull request Jun 27, 2017

[3.6] bpo-30674: IDLE: add docstrings to grep.py (GH-2213) (#2434)

39e501a

Patch by Cheryl Sabella (cherry picked from commit 65474b9)

csabella deleted the grep branch July 1, 2017 22:23



		class GrepDialog(SearchDialogBase):
		"Search Dialog for Find in Files."

		@@ -1,3 +1,8 @@
		"""Grep dialog for Find in File functionality.

Uh oh!

Conversation

csabella commented Jun 15, 2017

Uh oh!

mlouielu 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!

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!

Choose a reason for hiding this comment

Uh oh!

mlouielu commented Jun 15, 2017

Uh oh!

csabella commented Jun 15, 2017

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!

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 Jun 16, 2017

Uh oh!

mlouielu commented Jun 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants