Skip to content

gh-83847: IDLE - Refactor common code between hyperparser and editor#18539

Closed
csabella wants to merge 4 commits into
python:mainfrom
csabella:bpo39666
Closed

gh-83847: IDLE - Refactor common code between hyperparser and editor#18539
csabella wants to merge 4 commits into
python:mainfrom
csabella:bpo39666

Conversation

@csabella

@csabella csabella commented Feb 17, 2020

Copy link
Copy Markdown
Contributor
  • Factor out similar code between hyperparser.py and editor.py for creating a Parser instance that is used for analysis.
  • Modify tests to use new function as it had been inline in hyperparser before.

@terryjreedy terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve in principle, subject to more review and a couple of possible changes (so far).

Comment thread Lib/idlelib/editor.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'1.0', copied from the old code, seems wrong, as it is the Shell top, before the sign-in message, let alone the first prompt. If one hits Enter at the prompt, I would think that the code should be set to '\n'.

I need to add some prints to be sure of what happens in Shell and editor with various inputs.

@csabella csabella Feb 18, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the shell, it looks like it will always find the last console tag, which is marked from the beginning to the end of the prompt. For example, in a new shell window, typing >>> def hello( without pressing Enter shows r to be (3.0, 3.4).

I wasn't able to think of a scenario where the prompt would be missing in a real shell, so I'm not sure if the default of 1.0 would ever be used there. However, adding a print(r) and running the testsuite shows that the console tag isn't set for most tests, so it falls back to the default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the shell tests are unrealistic. Of course, with the prompt moved to a sidebar, we should be looking, I believe, for a mark.

@taleinat taleinat Oct 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, with the prompt moved to a sidebar, we should be looking, I believe, for a mark.

With the shell moved to a sidebar (PR GH-15474), there's still a "console" tag on the newline (\n) before the beginning of the current input block, making code such as this continue to "just work".

@taleinat taleinat Oct 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're refactoring this, I think it's a great opportunity to remove the shell-specific part of this code from the EditorWindow class and into PyShell.

Comment thread Lib/idlelib/idle_test/test_editor.py Outdated
@bedevere-bot

Copy link
Copy Markdown

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

@csabella

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@terryjreedy

Copy link
Copy Markdown
Member

To properly review this, I need to look at EditorWindow chunk, Hyperparser chunk, and new function side-by-side when I have more time. Also see reply on the issue re location of new function.

@terryjreedy terryjreedy changed the title bpo-39666: Refactor common code between hyperparser and editor gh-83847: IDLE - Refactor common code between hyperparser and editor Nov 2, 2022
@terryjreedy terryjreedy self-assigned this Nov 2, 2022
@terryjreedy

Copy link
Copy Markdown
Member

Closing because I currently prefer #20847 with the new function in hyperparser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants