| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
- and for `unrecoverable` exceptions
…2aU3e.rst Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
# Conflicts: # Lib/_pyrepl/reader.py
…gey-miryanov/cpython into pythongh-130698-pyrepl-ps1-exception
Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
…gey-miryanov/cpython into pythongh-130698-pyrepl-ps1-exception
|
I updated this PR because it had merge conflicts. Not sure what to do next though. |
Sorry, something went wrong.
|
@chris-eibl @tomasr8 Could you please take a look? Is there anything else I can do to move this PR forward? |
Sorry, something went wrong.
|
Had another look and agree that this is correct:
As pointed out in #131110 (comment), this stems from the fact that cPython only uses a primary and secondary prompt:
and we have to map that to ps1 ... ps4 used in the implementation (coming from PyPy). I still think that your new DEFAULT_PS* should not be used just in case of an Exception, but throughout the code base, i.e. be the single source of truth for the defaults. E.g. cpython/Lib/_pyrepl/simple_interact.py Lines 134 to 135 in 246ed23 can become ps1 = getattr(sys, "ps1", DEFAULT_PS1) ps2 = getattr(sys, "ps2", DEFAULT_PS3) and all the other places @tomasr8 and I have mentioned above? Since that mapping seems to be confusing, only using ps1 and ps2 would be preferable, but most likely out of scope for this PR. Because AFAIR PyPy sometimes downstreams our changes, most probably ditching ps3 and ps4 is just churn for them and us, but having the defaults at a single place helps everybody. |
Sorry, something went wrong.
I have no preference here. Maybe a warning would be nice, that can easily be added if others want it. Your PR definitely fixes the issue, so LGTM 🚀 |
Sorry, something went wrong.
|
@chris-eibl Thanks a lot for such detailed answer! Working on this ... |
Sorry, something went wrong.
|
I left only DEFAULT_PS1 and DEFAULT_PS2 and added MULTILINE_PS*. I think it should be a bit clearer. I'm not sure should we use DEFAULT_PS1 in the IDLE module, because it uses a slightly different prompt (>>>\n instead of >>> ). Also, I think we should leave as-is C modules, where >>> and ... used to pre-initialize ps1 and ps2. |
Sorry, something went wrong.
|
@chris-eibl Could you please take a look? |
Sorry, something went wrong.
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Ups, quite a splash radius. Let's see how others like it.
Yes, it is, but chances are high this is rejected or shall go into its own PR? Still unsure about @tomasr8's comment
Most probably it is better to leave them alone. The way cPython uses it, they are always overwritten in multiline_input, anyway. If we'd like to strive for a least minimal change, then all defaults can be removed again and just return an empty string (like the old REPL did): def __get_prompt_str(prompt: object) -> str:
try:
return str(prompt)
except (MemoryError, SystemError):
raise
except Exception:
return ""That way the user at least get's a hint that something is flaky by neither seeing their expected prompt nor the default one, since atm no warnings are printed here. And we don't reinvent the wheel, there is already precedence ... See also #130698 (comment) of @skirpichev in the issue. Sorry for the back and forth, but simpler might be better? Up to you, just stopping here and waiting for the REPL maintainers to chime in is also an option ... |
Sorry, something went wrong.
Yes, I left it as it was because we always rewrite all prompts in multiline_input.
Yes, I think returning to the old behavior is a good idea now. At least it's simple and minimal. I'll wait a while for the pyrepl maintainer's opinion. If I don't hear back from anyone, I'll implement the old behavior. Thanks! |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
| ps1 = getattr(sys, "ps1", DEFAULT_PS1) | ||
| ps2 = getattr(sys, "ps2", DEFAULT_PS2) |
There was a problem hiding this comment.
Shouldn't these also use __get_prompt_str?
Sorry, something went wrong.
There was a problem hiding this comment.
Looks like it.
Feel free to use this as a base or build your own solution from scratch.
Sorry, something went wrong.
| exec(startup_code, console.locals) | ||
|
|
||
| ps1 = getattr(sys, "ps1", ">>> ") | ||
| ps1 = getattr(sys, "ps1", DEFAULT_PS1) |
There was a problem hiding this comment.
Same here for __get_prompt_str
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
It fixes cases when ps1, ps2, ps3, ps4 raise exception from __str__.
This PR still misses tests for Reader.arg, but maybe you can review the whole solution - do I drive in the right direction?
Fixed for Reader.arg.