RFC: remove repl prompt prefix before parsing

[KristofferC]

It is common to encounter example code that is posted with the REPL prefix included (typically julia>). This occurs in manual, doc tests and in forum posts etc. Usually one wants to quickly run this code in the REPL but then has to manually remove all the prompt prefixes first which can be a bit annoying.

This PR simply checks if the statement being evaluated in the REPL starts with the REPL prefix and in that case removes it.

As an example pasting this code block:

julia> const x = 1;

julia> function foo(x)
           return x + 1
       end;

julia> type X
       x::Int
       end;

will be equivalent to pasting it without the julia> prefixes.

This is also implemented for the other REPL modes.

fixes #16383

[KristofferC]

cc @StefanKarpinski because of #16383 (comment)

I've considered in the past automatically detecting leading julia> and then stripping the indent from the following lines so so that you can just paste REPL transcripts back into the REPL and have them work. Not sure that's a good idea though and of course it doesn't help other REPL modes unless we handle them too (and switch to the named REPL mode automatically?).

This does not do the automatic REPL mode switch.

[Keno]

Maybe activate this only in the bracketed paste mode parser?

[KristofferC]

That seems like a good idea. I will wait a bit with updates to the PR until there is more feedback regarding doing this at all but will change it later if people are positive to the idea in general.

[omus]

Is the julia> prompt saved in the command history?

[StefanKarpinski]

Is the julia> prompt saved in the command history?

It's not, but the mode is, from which you can figure out what the prompt was.

[KristofferC]

Okay, I updated this to work in the bracket-paste-mode instead. This also has the effect that the julia> part is completely removed from the input before the REPL prints it or it is saved in history. So it should be equivalent to pasting the code without the prompts there at all.

Was it something like this you had in mind @Keno?

[StefanKarpinski]

Very nice. I think this is a pretty handy addition :)

[mbauman]

This is awesome. I've thought of doing this myself a few times but never got to it.

A potential addition (for a future PR) — treat any non-prompt-prefixed data within the same bracketed paste operation as doc-test style assertions.

[tkelman]

clean this up when finished, do block would be good

[KristofferC]

cleaning up now in the same way as done in the other tests in this file

[tkelman]

I don't see many other tests in this file creating or deleting things? If there were and they weren't using do blocks yet I'd go fix them (in a separate PR if needed).

edit: I somehow missed

julia/test/repl.jl

Lines 69 to 94 in 9fbe3b5

	origpwd = pwd()
	tmpdir = mktempdir()
	write(stdin_write, ";")
	readuntil(stdout_read, "shell> ")
	write(stdin_write, "cd $(escape_string(tmpdir))\n")
	readuntil(stdout_read, "cd $(escape_string(tmpdir))"[max(1,end-39):end])
	readuntil(stdout_read, realpath(tmpdir)[max(1,end-39):end])
	readuntil(stdout_read, "\n")
	readuntil(stdout_read, "\n")
	@test pwd() == realpath(tmpdir)
	write(stdin_write, ";")
	readuntil(stdout_read, "shell> ")
	write(stdin_write, "cd -\n")
	readuntil(stdout_read, origpwd[max(1,end-39):end])
	readuntil(stdout_read, "\n")
	readuntil(stdout_read, "\n")
	@test pwd() == origpwd
	write(stdin_write, ";")
	readuntil(stdout_read, "shell> ")
	write(stdin_write, "cd\n")
	readuntil(stdout_read, homedir()[max(1,end-39):end])
	readuntil(stdout_read, "\n")
	readuntil(stdout_read, "\n")
	@test pwd() == homedir()
	rm(tmpdir)
	cd(origpwd)

[KristofferC]

On Apple i get Evaluated: "/private/var/folders/gw/_2jq29095y7b__wtby9dg_5h0000gn/T/tmpq563HH" == "/var/folders/gw/_2jq29095y7b__wtby9dg_5h0000gn/T/tmpq563HH" test failure. Does anyone know why the private part is dropped from the return value of mktempdir()?

Isn't

t = mktempdir()
cd(t)
@test pwd() == t

supposed to work?

[tkelman]

I've seen that before. I think it's a symlink resolution thing.

[KristofferC]

Looks like the other repl test uses pwd() == realpath(tmpdir). Do you think that would help?

[KristofferC]

Tests added, tests passing, bikeshed away.

To get started.

• If the first statement in a paste block contains julia> should all of them require it? In my opinion, no, because then pasting something which contains parsable output can be annoying ex:

julia> 1+1
1

julia> "hej alla goa gubbar"
"hej alla goa gubbar"

This ties in with what @mbauman said that the unprefixed output could potentially be turned into tests but that whole discussion can be had later.

• Similarly, if the first statement does not contain a julia> prompt, should then the rest of the statements be parsed as normally? In my view, again, no, ex:

# Random comment
julia> 1+1

• Whitespace. Should it be ok to paste:

julia> 1+1

    julia> 2+2

In my view, yes because when you copy stuff it is easy to get some spurious extra tabs or spaces.

Anything else?

[KristofferC]

Actually,

pasting

julia> "hej alla goa gubbar"
"hej alla goa gubbar"

turns into

"hej alla goa gubbar"
"hej alla goa gubbar"

which gives the error

ERROR: cannot document the following expression:

"hej alla goa gubbar"

 in error(::String, ::String, ::Vararg{String,N}) at ./error.jl:22

[vtjnash]

We may want to use Base.samefile more for those sorts of tests, since it avoids questions of alternative normalizations and such.

[StefanKarpinski]

I think it's fine for this not to work well if you paste the result too since that's easy to avoid.

[KristofferC]

After playing around with this for a bit I would say that if you are in "prompt paste mode", simply ignoring everything that does not have a prompt is the nicest user experience because then you can paste arbitrary sections of the REPL / forum posts without having to scrub the output:

Available for play at: https://github.com/KristofferC/PimpMyREPL.jl ;)

[StefanKarpinski]

Love the package name :)

[johnmyleswhite]

I'm a little concerned that the package name subtly undermines our effort to be an inclusive community for all genders.

[tkelman]

XzibitHeardYouLikeREPLs.jl

[PallHaraldsson]

[I now see above comment (and https://github.com/KristofferC/PimpMyREPL.jl that works in 0.4) making my comment here kind of redundant.. another possibility would be to point to it in an error message..]

I saw this under: "New language features" not "Breaking changes"

Can this be backported? It just seems very handy (for beginners, and we want then to use 0.5 (or 0.4) and not 0.6?)

I'm not too concerned, but:

julia> julia=1
1

julia> julia>1
false

so strictly breaking (but only for the REPL).. In case people are worried, it could be off by default in older versions.. then not really helping beginners.. [I do not have commit privileges, let alone backport, could I help in some way in such matters, not sure how automated it is (if not default changed).]

I know theres a feature freeze, seems this could be an exception for 0.5.0 or 0.5.1?

[KristofferC]

Note that this is only active when you paste something, not when you type it. You even have to paste julia> 1 with the space for it to be active, both julia>1 and julia > 1 will work fine but yes, in those cases it will be "breaking".

About backporting, I don't think it should be backported with default on but as I said in #17599 (comment) defaulting it to off should be safe. I could easy make a backport PR where I disable it by default and fixup the documentation but not sure if it will be accepted.

[KristofferC]

It is annoying that Githubs markdown renderer don't respect trailing whitespaces inside a quoted code block.

[tkelman]

I'm generally against backporting features right now. Let people try this out for a while on master, keep track of any bugs with it that might need to be fixed, and we can reconsider later. Not worth introducing more changes to 0.5.0 for this IMO.

Since this is a REPL extension couldn't it even be enabled as a package?

[KristofferC]

It could and it is.

[StefanKarpinski]

This is a wonderful addition to the REPL behavior, btw.

[KristofferC]

Thanks, I like it too.

[PallHaraldsson]

"against backporting features right now", you mean backport for 0.5.1 (and maybe 0.4.x) would be ok (even with strictly breaking)? I'm ok with that, since the "break" is hardly real and only for the REPL.

This seems just that good, that we wouldn't want to wait for a year for 0.6 (or use a package).

[tkelman]

x-ref #17939 if we do decide to backport this later

[StefanKarpinski]

This feature is so nice and since it's purely interactive, I'd be in favor of backporting to 0.5.x.

[tkelman]

or it could be enabled as a package already, can't it? would that even work for users on 0.4?

this missed feature freeze - if we backport absolutely everything, what was the point of feature freeze then?

[KristofferC]

It is already enabled in a package that (should) work on 0.4. I'm fine with not back porting this and as @tkelman it did miss the feature freeze.

[tkelman]

for one of the later point releases, probably disabled by default there as suggested above, should be fine. I don't think we've had new nightlies with this in it yet.

[musm]

Just to confirm this isn't working for me on windows either

[KristofferC]

Yes, and the docs so says so. But there are many terminals it works on in Windows, the standard one just lacks certain features needed.

[StefanKarpinski]

This change is so nice that I really want to backport it to 0.5. How do people feel about that? I can't imagine backporting this breaking anyone's programs...

[tkelman]

Disabled by default but with an option to turn it on I could see doing.

[StefanKarpinski]

How can this possibly break someone's code?

[tkelman]

It's a feature and a behavior change, neither of which we generally backport. It would make 0.5.0 behave diifferently than 0.5.1 which is not good. And it's implemented in a package that works already on 0.5.0.

[StefanKarpinski]

There's a massive difference between changes that only affect the REPL and other changes.

[tkelman]

0.5.x releases should be completely interchangeable with one another if you aren't hitting a bug that was fixed. This isn't a bug fix. What's the problem with installing the package to get this behavior on 0.5?

[StefanKarpinski]

That's not an a priori principle. Describe to me the scenario where having this feature in a 0.5.1 REPL but not in a 0.5.0 REPL causes any serious problem for someone.

[tkelman]

Well sure, it's a stricter interpretation of semver without the 0.x.y anything-goes exception.

Say you're teaching a course or working on a project with multiple other people, if people are on a range of different 0.5.x releases you shouldn't rely on features ("copy-paste this snippet of code") that work for only a subset of students or collaborators. Backporting features is what the minor version will be for after we hit 1.0, before then I'd rather say "if you want this feature to work on 0.5.x, use the package."

[StefanKarpinski]

That's an incredibly thin reason not to backport this. In that scenario, a few students may not be able to paste into the REPL including the julia> prompt, in which case they can do what we've all been doing previously – paste just the input part. In that same scenario, if we don't backport this, then no one can paste including the julia> prompt. How is that better? I get the compatibility principle here, but being pointlessly dogmatic about it seems misguided.

[tkelman]

This is a feature, and it missed the 0.5 feature freeze. 0.5.0 working the same way as 0.5.1 is better than having an inconsistent feature set. No one can paste including the prompt on 0.5 unless they install the package, which works consistently no matter which point release of Julia you have installed.

[nalimilan]

I agree with @tkelman. Let's release 0.6 soon if we want to give users access the new features quickly. 0.5.x releases should be as boring as possible.