Fix so windows opens the default editor when `@edit` is used #8725

dhoegh · 2014-10-18T15:40:29Z

This fixes so windows can open source files with @edit macro using the default editor and I have added a notice to the user where to find the function when no line number info is sent to the editor.

nalimilan · 2014-10-18T16:31:43Z

Thanks! But this is actually still not completely correct (not your fault, the code was already wrong), as the files are not guaranteed to be in that place. It should instead use the DATAROOTDIR constant which is set in base/build_h.jl, i.e. joinpath(JULIA_HOME, DATAROOTDIR, "julia", "base", file). Would you do that?

dhoegh · 2014-10-18T16:44:43Z

No problem. I was a bit too quick with the pull-request. My actual quest was to make @edit work with the default windows text editor, and I have now added what is necessary to do that plus some info to the user about which line number the method is defined at when the line number is not sent to the editor. Please rephrase the message to what you feel is appropriate.

pao · 2014-10-18T17:35:22Z

I was a bit too quick with the pull-request.

We have no problem with "early and often!" If you are making a PR and you know that it's not ready yet, it's conventional to prefix the title with "WIP" (Work In Progress), but we certainly don't expect everything to be 100% ready when it's proposed.

Thanks for contributing!

dhoegh · 2014-10-18T17:38:25Z

Okay, I am finished now, until some of you guys come up with any wanted changes:)

dhoegh · 2014-10-18T18:04:50Z

It is now fixed:)

nalimilan · 2014-10-18T18:52:10Z

Thanks! Could you squash these commits into one (using git rebase -i)?

nalimilan · 2014-10-18T19:29:04Z

Looks like something went wrong. You have added commits instead of squashing them into one. Use git rebase -i and then once you're done git push --force to overwrite the old commits on the branch of the pull request.

nalimilan · 2014-10-18T19:30:44Z

Ah, and please rewrite the commit message so that it makes sense out of the context of this PR (just like you did in the original commit).

dhoegh · 2014-10-18T19:49:09Z

wow, I succeeded should I also squash the last one away? As you probably figured out I am not that good at git.

nalimilan · 2014-10-18T19:53:11Z

Yes, please!

dhoegh · 2014-10-18T19:57:36Z

Fixed it, then I got schooled in git + VI:)

nalimilan · 2014-10-18T20:02:31Z

Sorry, there still are two commits. :-)

dhoegh · 2014-10-18T20:06:45Z

Are there? 2da44c5 is the new commit as I see only se here.

nalimilan · 2014-10-18T20:10:19Z

OK, now it's fine. Let's wait for Travis to finish the tests.

dhoegh · 2014-10-18T20:12:51Z

It should be backported as well right?

Keno · 2014-10-18T21:37:05Z

base/interactiveutil.jl

@@ -24,6 +24,7 @@ function edit(file::String, line::Integer)
        f = find_source_file(file)
        f != nothing && (file = f)
    end
+    no_line_msg = "Uknown editor: no line number information passed.\nThe method is defined at line $line."


Slight typo.

printed to the user when it is not passed to the editor.

dhoegh · 2014-10-18T21:49:17Z

Fixed and squashed.

vtjnash · 2014-10-19T02:35:03Z

despite the failed name mangling that will probably result if the path has a space or quote character in it, this is probably OK to merge anyways. spawn should really have the UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS argument since this is going through the DOS API before it gets to the Shell API (and then to the Win32 API and then to the NT API and finally to the kernel)

dhoegh · 2014-10-19T06:26:46Z

@vtjnash how should the UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS be used? I could not find any documentation on it regarding spawn()

Fix so windows opens the default editor when `@edit` is used

ivarne · 2014-10-19T21:26:46Z

@dhoegh Backports are very welcome!

Anyone can submit pull requests against the release-0.3 branch, but if you don't have time, I've gotten some experience so I'll gladly do it for you. Unfortunately this PR doesn't include tests so I can't automatically test whether the issue is really fixed there as well.

dhoegh · 2014-10-20T05:32:41Z

Hi brk00 could you make the back port together with the rest of the changes you have in 8738? If you just ping me on the backport then I will check it is working:)

dhoegh · 2014-10-20T05:49:58Z

I have found one problem with backporting DATAROOTDIR is not defined in 0.3, any ideas?

nalimilan · 2014-10-20T07:30:16Z

I have found one problem with backporting DATAROOTDIR is not defined in 0.3, any ideas?

Are you sure? It's defined here: https://github.com/JuliaLang/julia/blob/release-0.3/base/Makefile#L59 But you need to write Base.DATAROOTDIR.

dhoegh · 2014-10-20T18:14:47Z

If I write in the interpreter:

julia> Base.DATAROOTDIR
ERROR: DATAROOTDIR not defined

and I also tried it in write Base. in the front in the function do not work either.

ivarne · 2014-10-20T18:20:21Z

@nalimilan Base.DATAROOTDIR was backported in aacd2fa, just after 0.3.1 was tagged and released, ~~so @dhoegh is right.~~

dhoegh · 2014-10-20T18:22:44Z

Thanks for the info @ivarne.

ivarne · 2014-10-20T18:23:35Z

But this has to be fixed with a new 0.3.X release, so you should use the latest release-0.3 commit to base the pull request on.

dhoegh · 2014-10-20T18:30:03Z

But should I also compile source? I haven't done that before and I would prefer if I could avoid it.

ivarne · 2014-10-20T18:43:52Z

Compiling Julia from source is usually a requirement when you want to do changes to Julia that aren't trivial typo fixes.

This fix looks like it is about on the boundary of trivial, so I think you'll likely be able to do it and have someone else test to ensure that it works.

It would be nice to note in a PR that you haven't really tested that it works, so that you motivate us to do a more thorough review before merging.

dhoegh · 2014-10-20T18:49:01Z

I had tested on 0.4 on a one day old nightly where I precompiled Base. So I had tested it I have just not compiled Julia from source.

vtjnash · 2014-10-20T18:58:29Z

On windows, you can test a patch to base by running the prepare-Julia-env.bat script after making the changes and deleting sys.ji

dhoegh · 2014-10-20T19:00:52Z

That was what I did, but I can't do it for 0.3.x since there is not made nightlies only 0.3.1.

nalimilan · 2014-10-20T20:32:26Z

If you prepare a PR against 0.3 we can test it for you if you want -- though compiling Julia from scratch is a valuable experience for your next PR. ;-)

StefanKarpinski · 2014-10-20T20:39:45Z

compiling Julia from scratch is a valuable experience for your next PR

Also, not hard. Clone and run make -j N where N is however many cores you feel like pegging during the build process. It will be done in a little while – a few hours on a slow machine with N = 1.

tkelman · 2014-10-20T20:41:35Z

He's on Windows. So have to be careful to set up an environment following the steps in https://github.com/JuliaLang/julia/blob/master/README.windows.md

Or you can do Cygwin-to-MinGW cross-compilation, which I still need to document. The required packages are listed here: https://github.com/staticfloat/julia-vagrant/pull/2/files

…efault editor + line number is printed to the user when it is not passed to the editor.

Backport of #8725 Fix so `@edit` works on windows with the default editor.

dhoegh changed the title ~~Fix find_source_file(file) to use joinpath~~ Fix so windows opens the default editor when @edit is used Oct 18, 2014

dhoegh force-pushed the fix_find_source_file branch from fc40e83 to d3892bb Compare October 18, 2014 19:46

dhoegh force-pushed the fix_find_source_file branch from d3892bb to 2da44c5 Compare October 18, 2014 19:56

Keno reviewed Oct 18, 2014
View reviewed changes

Fix so @edit works on windows with the default editor + line number is

572c69c

printed to the user when it is not passed to the editor.

dhoegh force-pushed the fix_find_source_file branch from fa684ec to 572c69c Compare October 18, 2014 21:48

JeffBezanson added a commit that referenced this pull request Oct 19, 2014

Merge pull request #8725 from dhoegh/fix_find_source_file

d22bc42

Fix so windows opens the default editor when `@edit` is used

JeffBezanson merged commit d22bc42 into JuliaLang:master Oct 19, 2014

ivarne added the backport pending label Oct 19, 2014

dhoegh added a commit to dhoegh/julia that referenced this pull request Oct 21, 2014

Backport of JuliaLang#8725 Fix so @edit works on windows with the d…

c8695e2

…efault editor + line number is printed to the user when it is not passed to the editor.

dhoegh mentioned this pull request Oct 21, 2014

Backport of #8725 Fix so @edit works on windows with the default editor. #8748

Merged

nalimilan added a commit that referenced this pull request Oct 21, 2014

Merge pull request #8748 from dhoegh/backport_of_#8725

8383227

Backport of #8725 Fix so `@edit` works on windows with the default editor.

nalimilan removed the backport pending label Oct 21, 2014

tkelman mentioned this pull request Oct 21, 2014

Improve windows shell mode; fix edit() function for windows #7108

Closed

Fix so windows opens the default editor when @edit is used #8725

Fix so windows opens the default editor when @edit is used #8725

Conversation

dhoegh commented Oct 18, 2014

nalimilan commented Oct 18, 2014

dhoegh commented Oct 18, 2014

pao commented Oct 18, 2014

dhoegh commented Oct 18, 2014

dhoegh commented Oct 18, 2014

nalimilan commented Oct 18, 2014

nalimilan commented Oct 18, 2014

nalimilan commented Oct 18, 2014

dhoegh commented Oct 18, 2014

nalimilan commented Oct 18, 2014

dhoegh commented Oct 18, 2014

nalimilan commented Oct 18, 2014

dhoegh commented Oct 18, 2014

nalimilan commented Oct 18, 2014

dhoegh commented Oct 18, 2014

Keno Oct 18, 2014

Choose a reason for hiding this comment

dhoegh commented Oct 18, 2014

vtjnash commented Oct 19, 2014

dhoegh commented Oct 19, 2014

ivarne commented Oct 19, 2014

dhoegh commented Oct 20, 2014

dhoegh commented Oct 20, 2014

nalimilan commented Oct 20, 2014

dhoegh commented Oct 20, 2014

ivarne commented Oct 20, 2014

dhoegh commented Oct 20, 2014

ivarne commented Oct 20, 2014

dhoegh commented Oct 20, 2014

ivarne commented Oct 20, 2014

dhoegh commented Oct 20, 2014

vtjnash commented Oct 20, 2014

dhoegh commented Oct 20, 2014

nalimilan commented Oct 20, 2014

StefanKarpinski commented Oct 20, 2014

tkelman commented Oct 20, 2014

Fix so windows opens the default editor when `@edit` is used #8725

Fix so windows opens the default editor when `@edit` is used #8725