Conversation
|
I'm sorry, but I don't understand why Appveyor is failing. It's on my new tests, which are like the existing tests, only on a Text box (so it's not attached to a tracer). The failed test is showing |
terryjreedy
left a comment
terryjreedy
left a comment
There was a problem hiding this comment.
This is a partial review, but at least see if the text code change solves the test failure.
I do not know yet if this patch, or indeed, IDLE startup, works correctly. I may want to do some refactoring in pyshell before or as part of this change. Will comment on issue.
Lib/idlelib/pyshell.py
Outdated
| self.compile.compiler.flags = self.original_compiler_flags | ||
| if idleConf.GetOption('main', 'ShellWindow', | ||
| 'restart-code-on', type='bool'): | ||
| config_startup_code = idleConf.GetOption( |
There was a problem hiding this comment.
Hmm, 'startup' seems fine here, perhaps because of the '_'s and the name being long anyway.
Lib/idlelib/configdialog.py
Outdated
| self.fontpage = FontPage(note, self.highpage) | ||
| self.keyspage = KeysPage(note) | ||
| self.genpage = GenPage(note) | ||
| self.startuppage = StartupPage(note) |
There was a problem hiding this comment.
I prefer 'startpage' and 'StartPage' for internal code, but leave 'Startup' on the tab itself.
| if idleConf.GetOption('main', 'ShellWindow', | ||
| 'startup-code-on', type='bool'): | ||
| config_startup_code = idleConf.GetOption( | ||
| 'main', 'ShellWindow', 'shell-startup-code') |
There was a problem hiding this comment.
I seems to have missed the difference between 'restart-code-on' and 'startup-code-on'. I need to look at pyshell changes, especially, in broader context. I want to understand and document the pyshell code better, with comments and docstrings , before changing it.
There was a problem hiding this comment.
The 'restart-code-on' and 'startup-code-on' are checkbuttons that indicate when to run the code defined within config. Although your comment on the ticket mentions just having one code set for it, since startup and restart are different now, I added the checkbutton so that the code could be run just at initial startup, just at restart (Ctrl-F6), or both.
I thought documenting and tests for pyshell might be added as a dependency, just as the editor tests project would be a dependency.
Lib/idlelib/pyshell.py
Outdated
| 'startup-code-on', type='bool'): | ||
| config_startup_code = idleConf.GetOption( | ||
| 'main', 'ShellWindow', 'shell-startup-code') | ||
| sys.argv = ['-c'] + [config_startup_code] |
There was a problem hiding this comment.
When I use '-c' in Windows command prompt, the string that follows must be quoted with "", not '' (not an issue when provided in a variable) and cannot contain literal newlines. I know that Linux shells are different. I suspect that the newlines in the test strings might be the test failure cause.
Even if the test passes, I would not be sure that this line will work as expected. User code must be executed by run.py in its pseudomain context after Python starts run.py in the real main context.
There was a problem hiding this comment.
It worked on Linux and I wasn't sure what issues it would run into on other systems. Although I was considering the config code to be similar to the '-c' or -'s' options in terms of functionality. But I didn't realize that those might not currently be correct.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
|
This PR is stale because it has been open for 30 days with no activity. |
hauntsaninja
changed the title
github-actions
bot
removed
the
stale
|
This PR is stale because it has been open for 30 days with no activity. |
5 participants
https://bugs.python.org/issue5594