bpo-5680: IDLE: Customize running a module. by csabella · Pull Request #13763 · python/cpython (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation53 Commits19 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
| cli_args = self.entry.get().strip() |
|---|
| if not cli_args: |
| self.showerror('no arguments specified.') |
| return None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO "Run Module" should always ignore the latest "Custom Run" settings, regardless of whether or not a subprocess is used.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Run Module is currently unchanged and agree that this should continue.
| return lex |
|---|
| def restart_ok(self): |
| return self.restartvar.get() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add edits here for when restart cannot be False.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer possible.
| _basename(_sys.argv[0]) != _basename(__file__)): |
|---|
| _sys.argv = [__file__] |
| _basename(_sys.argv[0]) != _basename(__file__) or |
| len(argv) > 1): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this can only matter if one used the deprecated -n option. I have no intention of changing F5
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
- 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.
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?
Thanks for making the requested changes!
@terryjreedy: please review the changes made to this pull request.
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.
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..."?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
| def test_blank_args(self): |
|---|
| dialog = self.Dummy_CustomRun(' ') |
| self.assertEqual(dialog.cli_args_ok(), None) |
| self.assertIn('no arguments', dialog.entry_error['text']) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use assertRegex to do a case-insensitive check.
| self.assertIn('no arguments', dialog.entry_error['text']) |
|---|
| self.assertRegex(r'(?i)no arguments', dialog.entry_error['text']) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert is gone.
| class Dummy_CustomRun: |
| cli_args_ok = query.CustomRun.cli_args_ok # Function being tested. |
| entry = Var() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a class attribute? Besides being unclear, this also doesn't reflect the actual CustomRun class.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| class Dummy_CustomRun: |
|---|
| cli_args_ok = query.CustomRun.cli_args_ok # Function being tested. |
| entry = Var() |
| entry_error = {} |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| for dialog.restart in {True, False}: |
|---|
| for cli_args, result in ((None, None), |
| ('my arg', ('my arg', dialog.restart))): |
| with self.subTest(): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| with self.subTest(): |
|---|
| with self.subTest(restart=dialog.restart, cli_args=cli_args): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| @@ -0,0 +1 @@ |
|---|
| Run modules from the editor using command line arguments. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a bit more explicit, mentioning that a new menu option is added.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expanded.
| dialog = query.CustomRun(root, 'Title', _utest=True) |
|---|
| dialog.entry.insert(0, 'okay') |
| dialog.button_ok.invoke() |
| self.assertEqual(dialog.result, (['okay'], True)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| dialog.button_ok.invoke() |
|---|
| self.assertEqual(dialog.result, (['okay'], True)) |
| del dialog |
| root.destroy() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| dialog.entry.insert(0, 'okay') |
|---|
| dialog.button_ok.invoke() |
| self.assertEqual(dialog.result, (['okay'], True)) |
| del dialog |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are this del call, and the one below, actually needed?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Gone. Added class attributes need deletion, but function locals already go on return, and there is no evident issue here with destroying first.
| self.showerror('no arguments specified.') |
|---|
| return None |
| try: |
| lex = shlex.split(cli_args, posix=True) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does lex stand for? For me, this is confusing both here and in entry_ok(). Perhaps args_list?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I switched to cli_string and cli_args. cli_args_ok should return cli_args.
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.
| cli_args = self.entry.get().strip() |
|---|
| if not cli_args: |
| self.showerror('no arguments specified.') |
| return None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Run Module is currently unchanged and agree that this should continue.
| # User cancelled. |
|---|
| if not run_args: |
| return 'break' |
| return self._run_module_event(event, cli_args=run_args[0], restart=run_args[1]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to pass run_args and unpack in _run_module.
| self.showerror('no arguments specified.') |
|---|
| return None |
| try: |
| lex = shlex.split(cli_args, posix=True) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I switched to cli_string and cli_args. cli_args_ok should return cli_args.
| return lex |
|---|
| def restart_ok(self): |
| return self.restartvar.get() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer possible.
| for dialog.restart in {True, False}: |
|---|
| for cli_args, result in ((None, None), |
| ('my arg', ('my arg', dialog.restart))): |
| with self.subTest(): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| dialog = query.CustomRun(root, 'Title', _utest=True) |
|---|
| dialog.entry.insert(0, 'okay') |
| dialog.button_ok.invoke() |
| self.assertEqual(dialog.result, (['okay'], True)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| dialog.entry.insert(0, 'okay') |
|---|
| dialog.button_ok.invoke() |
| self.assertEqual(dialog.result, (['okay'], True)) |
| del dialog |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Gone. Added class attributes need deletion, but function locals already go on return, and there is no evident issue here with destroying first.
| dialog.button_ok.invoke() |
|---|
| self.assertEqual(dialog.result, (['okay'], True)) |
| del dialog |
| root.destroy() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
bpo-5680: IDLE: Run modules with command line arguments bpo-5680: IDLE: Customize running a module.
Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
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 cheryl.sabella@gmail.com
miss-islington added a commit that referenced this pull request
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 cheryl.sabella@gmail.com
miss-islington added a commit that referenced this pull request
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 cheryl.sabella@gmail.com
@taleinat, thank you for the review and @terryjreedy, thank you for the review and making the additional changes.
lisroach pushed a commit to lisroach/cpython that referenced this pull request
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.
DinoV pushed a commit to DinoV/cpython that referenced this pull request
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.