Feature/add line numbers to prompt #14223
Conversation
and then just I recommend using the |
|
Sorry to be a bother, but I have followed those steps and still the tests are failing, even on Here are my commands, from the root directory, with relevant output: So it seems to me like some necessary packages are still not getting installed. What am I doing wrong? |
|
pyest seem to not pick up its config in pyproject.toml, not sure why. I've pushed some commit that shoudl make the test pass in CI. |
|
I've also made it a tad more generic, with a template parameter for the line numbering. |
|
Also I'll let you look at the function |
cohml
force-pushed
the
feature/add-line-numbers-to-prompt
branch
from
9675993
to
29315b8
Compare
|
@Carreau -- I'm finally able to get back to this, but am confused by what I see wrt these failing tests. The build log shows that the first failure is in In fact, out of fully 1303 tests just a single one is failing for me locally (and, incidentally, I checked and it was also failing on Do you have any insights? This is my first exposure to GitHub builds so perhaps there are variables I'm unaware of when it comes to reproducibility... For context, all outputs above were generated with a fresh clone of the repository created using the same commands from my earlier comment, though thanks to you they now seem to work. |
|
To add further confusion, when I run that (single) failing test module directly, it actually passes, both on my branch and on I really want to get these changes merged, especially since they're useful and a (in theory) relatively simple addition to this otherwise awesome package. But I admit, the challenge in simply running these tests is seriously dampening the enthusiasm :( What do you suppose I'm doing wrong?? |
|
Don't worry about the failures, they are on Python 3.13-dev, this means that IPython has incompatibility with the next version of Python, it is likely changes in Python internal APIs, and it's either to warn us – core dev – that we need to update the API we use, or to report a bug upstream in CPython. |
|
Haha, if you are telling me to ignore the failing tests, then ignore the failing tests I will 🤣 In that case, the fix for emacs mode seems relatively straightforward. Thanks for pointing me to |
|
Paging @Carreau - Let me know if there's any additional work you'd like me to put into this, versus if you think it's good enough to merge as is, in light of your "don't worry about the failures" comment. I'd love to finally get some line numbers into my iPython shell! |
Carreau
changed the title
|
Thanks for your patience, I pushed a commit that just reformat a bit to please the linter and will merge a bit later once the PyPy test are passing. It should be in the next release. |
|
Glorious! IPython just got even better if that was even possible 😄 Thanks for the help and pleasure doing business with you! |
|
This PR breaks backward compatibility for prompts other than the one you just patched. In particular, it completely breaks sagemath. Example 1:Just run This is because you patched the base class but not the However, patching Example2:Since sagemath uses custom prompts, it's completely broken. Just run According to https://ipython.readthedocs.io/en/stable/config/details.html#custom-prompts you should not call Maybe you can add a flag in the prompt class meaning that For the time being, it would be nice if this is reverted until it's fixed, and 8.19.1 is released with this revert. |
This could probably be determined by using inspect module. |
|
Yeah, I'll see if I can make it work with inspect, or using something like https://pypi.org/project/backcall/. PR welcome though, and sorry for the breakage. |
Ditto, apologies for the trouble, all. At first I had attempted to keep my changes backwards compatible using Perhaps in retrospect that was the wrong decision! |
|
Yeah, I though that was unnecessary as I couldn't' find project using it. I have the fix locally (I also realized some of our docs were wrong since the method also changed in 2017 when prompt toolkit was updated to 2.0, I'll try to get that in soonish, but I'm travelling and on holidays, so it might take some time to actually be published. |
In the comment by @Carreau it clealy suggests some code and explains: "...that will check that lineno is a parameter and if not don't pass it to not break backward compatibility." How do you read "backwards compatibility wasn't required for this" from that?
As a learning experience, if you had taken the trouble to document your changes, maybe you would have realized this was part of a public api, and try to figure out what's a good way to move the api forward without breaking existing users of the api. In any case, changing an api without changing the documentation is not very friendly. The net result of this is reinforcing sagemath stupid habit of setting upper bounds on package versions (see: sagemath/sage#36975 (comment)). IMHO the best way to get out of this mess would be to revert this PR and release 8.19.1 asap. Then figure out a new api for Why not just add **kwargs to the interface? This would be as extensible as it gets... Then you can pass Maybe changing the name of the method for the new api is easier than using inspect? The base class could have a default implementation that falls back to the previous api. Then the way to signal your prompt class implements the new api is to implement the new method (overriding the base class default one). |
|
In the same thread I said later:
So this is my fault for doubting there was anybody using custom prompt and not doing complete testing. I also merged the PRs after reviews, and I saw that this was backward incompatible, so it's on me, not on @cohml #14274 will fix it, no need to revert, and yes it would be great to ask users to take In general I'm also worried about variability of what is passed when. Also please don't add upperbound, just add a I'll do a 8.19.1 when I can. |
Keep in mind sagemath is a big user of ipython since the beginning. In fact I believe ipython + cython (then called pyrex) were the two main reasons @williamstein chose python for SAGE back in 2004/2005.
If you mean removing the I can make a PR to move forward sagemath to the new api once is well defined and documented, but it'd be nice if the old api is supported for some time (say, 6 months should be ok). |
Well, no not really, as we call the method since 2017 with It just happens to not use the parameters. I have no issue having a long-term inspect that only passes the arguments defined in the underlying callback signature as long as it is documented why. I tend to dislike doing it for no reasons. |
Uh... I see..
Why not copy the prompt_toolkit api, i.e. have a method |
|
Confirmed, plus the python language itself had the biggest overlap with Magma’s language. |
|
Let's stay on task for now, fix the breaking backward compatibility and make a release quickly. I probably won't do a release before Sunday as I am on a connection that is barely enough to browse. After we can talk about improving things if we wish. If possible let's talk in #14273 that tracks this regression, and please review #14274 |
I've already patched our package for 8.19.0 in void linux, so not a big hurry on our side. I'll try to have sagemath not ban >=8.19 but only 8.19.0.
Sure, again if you let me know about the new api I can make a PR for sagemath to adapt.
Done. Thanks for the prompt handling of the issue. |
|
I just did a release the should fix the issue and yanked 8.19. |
Adding a lightweight test with SageMath using the modularized distribution https://pypi.org/project/sagemath-repl/ of the Sage library. Motivated by sagemath/sage#36975, #14223 (comment), sagemath/sage#36993, sagemath/sage#37031 @Carreau
This PR adds line numbers to the default prompt (#13965).
Emacs mode preview:
Vim mode preview:
Note: These changes are untested for now. I am struggling to run the existing tests in my dev environment. Are there any instructions on how to get everything set up for testing purposes?