bpo-5680: IDLE: Customize running a module. by csabella · Pull Request #13763 · python/cpython

csabella · 2019-06-03T00:37:04Z

The initialize options are 1) add command line options, which are appended to sys.argv as if passed on a real command line, and 2) skip the shell restart. The customization dialog is accessed by a new item on the Run menu.

https://bugs.python.org/issue5680

Lib/idlelib/query.py

csabella · 2019-06-03T22:37:23Z

Lib/idlelib/query.py

+        cli_args = self.entry.get().strip()
+        if not cli_args:
+            self.showerror('no arguments specified.')
+            return None


Not sure about this edit, but it makes everything a little nicer right now. The reason I'm not sure about it is due to the behavior of the current Run Module. Should Run Module use the last settings from Custom Run or should it work exactly as it works now (while in a subprocess)? Run Module without a subprocess uses the Custom Run settings from the last run.

IMO "Run Module" should always ignore the latest "Custom Run" settings, regardless of whether or not a subprocess is used.

I believe Run Module is currently unchanged and agree that this should continue.

csabella · 2019-06-03T22:38:09Z

Lib/idlelib/query.py

+        return lex
+
+    def restart_ok(self):
+        return self.restartvar.get()


Could add edits here for when restart cannot be False.

No longer possible.

csabella · 2019-06-03T22:43:31Z

Lib/idlelib/runscript.py

-                _basename(_sys.argv[0]) != _basename(__file__)):
-                _sys.argv = [__file__]
+                _basename(_sys.argv[0]) != _basename(__file__) or
+                len(argv) > 1):


Checking the length so that, if there are additional args, they will replace what was already there. However, existing args won't be replaced by no args. Currently, F5 still runs with no args and restarts the shell, but that's something to keep in mind if F5 changes.

I believe that this can only matter if one used the deprecated -n option. I have no intention of changing F5

terryjreedy

Test of behavior without looking much at code yet. Will respond to other comments later. Immediate issue: I have a custom keyset without new pseudoevent binding and on startup see

Warning: config.py - IdleConf.GetCoreKeys -
problem retrieving key binding for event '<>'
from key set 'test'.
returning default value: ['']

The fix is simple and I know how to do it if you don't beat me.

Separate issue if not done yet: write back keyset with new bindings.

Otherwise, works great.

Lib/idlelib/query.py

bedevere-bot · 2019-06-06T06:01:17Z

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

taleinat · 2019-06-06T17:57:11Z

I find the name "Run Custom" unclear and confusing, and I image some users would too. Is "Run Module With Arguments" too long? Perhaps "Run With Arguments..."?
It would be great to remember the last used arguments for each file, and pre-populate the field with them.

csabella · 2019-06-06T21:57:52Z

I have made the requested changes; please review again.

@terryjreedy, I added this pseudoevent to the former_extension_events set. I think that's what you wanted to fix -- to suppress the warning for the new binding and then to work on updating any custom keysets in another ticket. Maybe former_extension_events needs to be renamed?

bedevere-bot · 2019-06-06T21:57:54Z

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

csabella · 2019-06-06T22:07:24Z

Thanks for looking at this, @taleinat!

I find the name "Run Custom" unclear and confusing, and I image some users would too. Is "Run Module With Arguments" too long? Perhaps "Run With Arguments..."?

Terry wants to have more things on that dialog to be able to change the run environment, so using 'arguments' on the menu isn't inclusive enough. If 'Run Custom' is awkward, maybe 'Customize Run'?

It would be great to remember the last used arguments for each file, and pre-populate the field with them.

Yes, there was a few things that I listed on the bpo issue that I didn't do in this first round, including saving the values. Terry requested creating a minimal version to start with. Saving the items wouldn't (shouldn't?) complicate the code, but I wanted to try to keep it simple and then iterate over it in the next round. He had also mentioned printing the run options in the shell window so that multiple runs would be annotated. Both features would be helpful.

taleinat · 2019-06-07T06:14:13Z

I find the name "Run Custom" unclear and confusing, and I image some users would too. Is "Run Module With Arguments" too long? Perhaps "Run With Arguments..."?

Terry wants to have more things on that dialog to be able to change the run environment, so using 'arguments' on the menu isn't inclusive enough. If 'Run Custom' is awkward, maybe 'Customize Run'?

I see. Perhaps just "Run..."?

taleinat

As a general note, I'm not sure about the usefulness of the non-GUI tests for the RunCustom dialog (CustomRunCLIargsokTest, CustomRunEntryokTest) . They rely rather heavily on the internal implementation. Since we have GUI tests run by the CI now, I prefer to have simpler tests actually using Tk with less mocking. Would it be possible to simply move the tests from these classes into CustomRunGuiTest?

taleinat · 2019-06-11T08:20:25Z

Lib/idlelib/idle_test/test_query.py

+    def test_blank_args(self):
+        dialog = self.Dummy_CustomRun(' ')
+        self.assertEqual(dialog.cli_args_ok(), None)
+        self.assertIn('no arguments', dialog.entry_error['text'])


This should use assertRegex to do a case-insensitive check.

Suggested change

self.assertIn('no arguments', dialog.entry_error['text'])

self.assertRegex(r'(?i)no arguments', dialog.entry_error['text'])

This assert is gone.

Lib/idlelib/idle_test/test_query.py

taleinat · 2019-06-11T08:25:51Z

Lib/idlelib/idle_test/test_query.py

+
+    class Dummy_CustomRun:
+        cli_args_ok = query.CustomRun.cli_args_ok  # Function being tested.
+        entry = Var()


Why is this a class attribute? Besides being unclear, this also doesn't reflect the actual CustomRun class.

I wrote the query tests, and Cheryl followed the pattern I set. For testing with one instance at a time, or ever, class and instance attributes work the same. For attributes never rebound, I attached them to the class, especially if there was no other need for an init method. Very clear to me. However, I moved Var attributes to init when there is one as it saves a line.

The cli_args_ok method being tested accesses self.query. tk Vars and Entries both have a get method that does the same thing, so a mock Var replaces an Entry quite nicely.

taleinat · 2019-06-11T08:29:27Z

Lib/idlelib/idle_test/test_query.py

+    class Dummy_CustomRun:
+        cli_args_ok = query.CustomRun.cli_args_ok  # Function being tested.
+        entry = Var()
+        entry_error = {}


It might be simpler to make Dummy_CustomRun a Mock instance and then check calls to showerror(), rather than the entry_error['text'] stuff which is more implementation-specific.

I used this in all the other functional method tests. The dummy classes have and do exactly what is needed to test the method. A unittest.mock would be much more complicated and difficult and would run much slower. Test speed is important.

taleinat · 2019-06-11T08:31:35Z

Lib/idlelib/idle_test/test_query.py

+        for dialog.restart in {True, False}:
+            for cli_args, result in ((None, None),
+                                     ('my arg', ('my arg', dialog.restart))):
+                with self.subTest():


Suggested change

with self.subTest():

with self.subTest(restart=dialog.restart, cli_args=cli_args):

taleinat · 2019-06-11T08:46:34Z

Misc/NEWS.d/next/IDLE/2019-06-03-00-39-29.bpo-5680.VCQfOO.rst

@@ -0,0 +1 @@
+Run modules from the editor using command line arguments.


This needs to be a bit more explicit, mentioning that a new menu option is added.

taleinat · 2019-06-11T08:49:51Z

Lib/idlelib/idle_test/test_query.py

+        dialog =  query.CustomRun(root, 'Title', _utest=True)
+        dialog.entry.insert(0, 'okay')
+        dialog.button_ok.invoke()
+        self.assertEqual(dialog.result, (['okay'], True))


The should probably be a call to root.update() after the .invoke() call, to ensure that Tk has a chance to do all of its processing.

I believe invoke blocks until done. I have run this test at least 20 times without issue. event_generate is different (and flakey). There are 55 invokes in idle tests and my spot check of nearly 10 showed none followed by update. And I know of no resulting failures.

taleinat · 2019-06-11T08:52:14Z

Lib/idlelib/idle_test/test_query.py

+        dialog.button_ok.invoke()
+        self.assertEqual(dialog.result, (['okay'], True))
+        del dialog
+        root.destroy()


I suggest cleaning up using self.addCleanup() immediately after creating the Tk() instance, e.g.:

root = Tk() def cleanup(): root.update() root.destroy() self.addCleanup(cleanup)

I far prefer explicit calls. Update is not generally needed. update_idletasks is more often needed. I have not seen any of the tclerror messages that so indicate.

taleinat · 2019-06-11T08:52:50Z

Lib/idlelib/idle_test/test_query.py

+        dialog.entry.insert(0, 'okay')
+        dialog.button_ok.invoke()
+        self.assertEqual(dialog.result, (['okay'], True))
+        del dialog


Are this del call, and the one below, actually needed?

No. Gone. Added class attributes need deletion, but function locals already go on return, and there is no evident issue here with destroying first.

taleinat · 2019-06-11T08:54:53Z

Lib/idlelib/query.py

+            self.showerror('no arguments specified.')
+            return None
+        try:
+            lex = shlex.split(cli_args, posix=True)


What does lex stand for? For me, this is confusing both here and in entry_ok(). Perhaps args_list?

Agreed. I switched to cli_string and cli_args. cli_args_ok should return cli_args.

terryjreedy · 2019-06-16T00:36:02Z

I am reviewing and editing with the intention of getting this into 3.7.4. While the user UI can be changed after that, it is my primary focus. Summary responses to comments above.

Config warning: fixed.
Menu entry: We could flip 'Run custom', but I prefer to keep to a verb form. ('Python Shell' might better be 'Open Shell'.) I think 'Run customized' or 'Run ... customized' as an abbreviation for 'Run module with customized settings' is better, and should be good enough for now. I will see how latter looks.
Restart (or not) button: now functional. Add to blurb.
Arguments, without restart: These extend sys.argv. 'Command line' arguments do not make sense without restart, but extending sys.argv does. So this is OK.
No arguments, with restart: I changed my mind and will remove the current error check. No args will be ok with other settings, and if someone opens box to see available customizations, I now think they should be able to run without changing anything.
(Note: check button will be replaced with radio buttons when another mutually exclusive option is added, such as 'O Run module in system console' (no issue yet).)
Code comments, especially tests: I will make the changes I currently agree with.
Blurb: will expand.

Revise corresponding doc entry.

Lib/idlelib/query.py

terryjreedy · 2019-06-16T04:04:53Z

Lib/idlelib/query.py

+        cli_args = self.entry.get().strip()
+        if not cli_args:
+            self.showerror('no arguments specified.')
+            return None


I believe Run Module is currently unchanged and agree that this should continue.

terryjreedy · 2019-06-16T04:05:57Z

Lib/idlelib/runscript.py

+        # User cancelled.
+        if not run_args:
+            return 'break'
+        return self._run_module_event(event, cli_args=run_args[0], restart=run_args[1])


Changed to pass run_args and unpack in _run_module.

terryjreedy · 2019-06-16T04:28:57Z

Lib/idlelib/query.py

+            self.showerror('no arguments specified.')
+            return None
+        try:
+            lex = shlex.split(cli_args, posix=True)


Agreed. I switched to cli_string and cli_args. cli_args_ok should return cli_args.

terryjreedy · 2019-06-16T04:30:10Z

Lib/idlelib/query.py

+        return lex
+
+    def restart_ok(self):
+        return self.restartvar.get()


No longer possible.

Lib/idlelib/idle_test/test_query.py

terryjreedy · 2019-06-16T06:17:51Z

Lib/idlelib/idle_test/test_query.py

+        for dialog.restart in {True, False}:
+            for cli_args, result in ((None, None),
+                                     ('my arg', ('my arg', dialog.restart))):
+                with self.subTest():


terryjreedy · 2019-06-16T06:25:35Z

Lib/idlelib/idle_test/test_query.py

+        dialog =  query.CustomRun(root, 'Title', _utest=True)
+        dialog.entry.insert(0, 'okay')
+        dialog.button_ok.invoke()
+        self.assertEqual(dialog.result, (['okay'], True))


I believe invoke blocks until done. I have run this test at least 20 times without issue. event_generate is different (and flakey). There are 55 invokes in idle tests and my spot check of nearly 10 showed none followed by update. And I know of no resulting failures.

terryjreedy · 2019-06-16T06:27:45Z

Lib/idlelib/idle_test/test_query.py

+        dialog.entry.insert(0, 'okay')
+        dialog.button_ok.invoke()
+        self.assertEqual(dialog.result, (['okay'], True))
+        del dialog


No. Gone. Added class attributes need deletion, but function locals already go on return, and there is no evident issue here with destroying first.

terryjreedy · 2019-06-16T06:30:41Z

Lib/idlelib/idle_test/test_query.py

+        dialog.button_ok.invoke()
+        self.assertEqual(dialog.result, (['okay'], True))
+        del dialog
+        root.destroy()


I far prefer explicit calls. Update is not generally needed. update_idletasks is more often needed. I have not seen any of the tclerror messages that so indicate.

terryjreedy · 2019-06-16T23:33:52Z

buglet to fix: focus is not being shifted to shell correctly.

Don't ask for config if still may toss. Want focus to go to shell unless cancel.

terryjreedy · 2019-06-18T01:55:31Z

Changes: 1) Check code before bother to customize. 2) Add module title to title bar. 3) Make shell.text the parent, so focus goes to shell. Good enough to merge. See issue for 2 remaining issues.

miss-islington · 2019-06-18T02:24:40Z

Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2019-06-18T02:24:40Z

Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

The initialize options are 1) add command line options, which are appended to sys.argv as if passed on a real command line, and 2) skip the shell restart. The customization dialog is accessed by a new entry on the Run menu. (cherry picked from commit 201bc2d) Co-authored-by: Cheryl Sabella <[email protected]>

bedevere-bot · 2019-06-18T02:24:57Z

GH-14184 is a backport of this pull request to the 3.7 branch.

bedevere-bot · 2019-06-18T02:25:23Z

GH-14185 is a backport of this pull request to the 3.8 branch.

The initialize options are 1) add command line options, which are appended to sys.argv as if passed on a real command line, and 2) skip the shell restart. The customization dialog is accessed by a new entry on the Run menu. (cherry picked from commit 201bc2d) Co-authored-by: Cheryl Sabella <[email protected]>

csabella · 2019-06-20T22:00:20Z

@taleinat, thank you for the review and @terryjreedy, thank you for the review and making the additional changes.

The initialize options are 1) add command line options, which are appended to sys.argv as if passed on a real command line, and 2) skip the shell restart. The customization dialog is accessed by a new entry on the Run menu.

bpo-5680: IDLE: Run modules with command line arguments

ffc22b8

csabella requested a review from terryjreedy as a code owner June 3, 2019 00:37

the-knights-who-say-ni added the CLA signed label Jun 3, 2019

bedevere-bot added the awaiting core review label Jun 3, 2019

blurb-it bot and others added 2 commits June 3, 2019 00:39

📜🤖 Added by blurb_it.

36f7771

Fix editing error

69cf1b6

brettcannon added the type-feature A feature request or enhancement label Jun 3, 2019

Create more generic dialog and change keybinding

dbdd580

csabella commented Jun 3, 2019

View reviewed changes

Lib/idlelib/query.py Show resolved Hide resolved

csabella commented Jun 3, 2019

View reviewed changes

Lib/idlelib/query.py Show resolved Hide resolved

csabella commented Jun 3, 2019

View reviewed changes

Forkest mentioned this pull request Jun 4, 2019

bpo-5680: IDLE: Add run module with arguments to Key-F7 in editor #1589

Closed

terryjreedy requested changes Jun 6, 2019

View reviewed changes

Lib/idlelib/query.py Show resolved Hide resolved

Lib/idlelib/query.py Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting core review labels Jun 6, 2019

Don't show warning message for new key binding

f1e7c61

bedevere-bot removed the awaiting changes label Jun 6, 2019

bedevere-bot added the awaiting change review label Jun 6, 2019

taleinat reviewed Jun 11, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into pr_13763

286e4c6

Change menu entry 'Run Custom' to 'Run ... Customized'.

03e0f59

Revise corresponding doc entry.

terryjreedy added 3 commits June 16, 2019 01:59

Move some test dummy attributes to instance.

845178b

A couple more.

31416fd

Update query coverage.

fbfca23

terryjreedy approved these changes Jun 16, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jun 16, 2019

pass rub_args by keyword

4da67c3

terryjreedy added 3 commits June 17, 2019 18:23

Merge branch 'master' into bpo5680

307ac6a

Show customize box after check, attach to shell.

4a64274

Don't ask for config if still may toss. Want focus to go to shell unless cancel.

Minor tweaks.

18177da

terryjreedy changed the title ~~bpo-5680: IDLE: Run modules with command line arguments~~ bpo-5680: IDLE: Customize running a module. Jun 18, 2019

terryjreedy merged commit 201bc2d into python:master Jun 18, 2019

bedevere-bot removed the awaiting merge label Jun 18, 2019

terryjreedy added needs backport to 3.7 labels Jun 18, 2019

bedevere-bot removed the needs backport to 3.7 label Jun 18, 2019

bedevere-bot removed the needs backport to 3.8 label Jun 18, 2019

csabella deleted the bpo5680 branch June 20, 2019 21:59

	self.assertIn('no arguments', dialog.entry_error['text'])
	self.assertRegex(r'(?i)no arguments', dialog.entry_error['text'])

	with self.subTest():
	with self.subTest(restart=dialog.restart, cli_args=cli_args):

		@@ -0,0 +1 @@
		Run modules from the editor using command line arguments. No newline at end of file

Uh oh!

Conversation

csabella commented Jun 3, 2019 • edited by terryjreedy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

csabella Jun 3, 2019 • 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!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jun 6, 2019

Uh oh!

taleinat commented Jun 6, 2019

Uh oh!

csabella commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jun 6, 2019

Uh oh!

csabella commented Jun 6, 2019

Uh oh!

taleinat commented Jun 7, 2019

Uh oh!

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

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!

Choose a reason for hiding this comment

csabella commented Jun 3, 2019 •

edited by terryjreedy

Loading

csabella Jun 3, 2019 •

edited

Loading

csabella commented Jun 6, 2019 •

edited

Loading