Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let workspace() call load_juliarc() #12314

Closed

Conversation

tomasaschan
Copy link
Member

When exploring some possibilities to optimize my workflow (see this thread on julia-users) I ran into problems because ~/.juliarc.jl wasn't loaded when I ran workspace(). This PR mitigates that.

If there is a reason not to do this, it's no big deal for me - I can always do workspace(); Base.load_juliarc() manually - but I think it feels both intuitive and useful that the startup script is run in every workspace, and not just the first in each session.

@yuyichao
Copy link
Contributor

This should only be done if the init process loads .juliarc.jl

@tomasaschan
Copy link
Member Author

@yuyichao I'm not familiar enough with the internals and bootstrapping of a Julia session to understand exactly what you're talking about. Are you saying that this is a bad idea altogether, or just that the load needs to be conditional inside workspace()? If the latter, how do I formulate the condition "if the init process loads .juliarc.jl" in code? :)

@yuyichao
Copy link
Contributor

I think this is a good idea and I'm saying it should be conditioned.

You can do a git grep juliarc to figure out what is the logic to load the startup file. (seems like JLOptions().startupfile != 2 ?)

@mbauman
Copy link
Sponsor Member

mbauman commented Jul 26, 2015

That option looks to be a little confusing. I think 0 is the default (on), 1 is explicitly on, and 2 is explicitly off. src/julia.h:1551

@tkelman
Copy link
Contributor

tkelman commented Jul 26, 2015

Would be nice if the option value defines were more easily accessible to Julia code to make this harder to break if the meaning of the option values ever changes.

One way to help with that would to construct a test that ensures this works exactly as intended as part of test/cmdlineargs.jl

@davidagold
Copy link
Contributor

Question: would calling workspace in the middle of testing somehow be a problem? Given the current coverage, it's not clear to me that it's been attempted.

@tkelman
Copy link
Contributor

tkelman commented Jul 26, 2015

We probably need to check the options for whether --code-coverage is being used and pass that on to the sub-process in https://github.com/JuliaLang/julia/blob/295bf63a522cdaf1847389c4882edffebe1a450d/test/workspace.jl

@tomasaschan
Copy link
Member Author

While I wholeheartedly agree that writing tests for this is a Good Thing(TM), actually doing it is a little outside of my depth (and, likely, requires me to invest more time than I have to learn what I need to get it done...) - there are too many moving parts that I know next to nothing about. After all, managing to suggest this change in the first place was mostly due to some luck with search terms :)

If someone wants to guide me through the process (and by that I probably mean more or less tell me what code to insert where) I can add tests to this PR.

If not, I'm totally fine with this not being merged until someone has the time to write some tests for it. After all, the workaround very simple.

@tomasaschan
Copy link
Member Author

(For anyone coming here looking for a workaround: I currently have the following in my .juliarc.jl, which does exactly what I want:)

function workspace()
    Base.workspace()
    Base.load_juliarc()
    nothing
end

@tkelman
Copy link
Contributor

tkelman commented Jul 29, 2015

I think this could be fine to merge now if others agree, my annoyance at hardcoded option enums and lack of testing aren't strong enough to hold up the change.

@tomasaschan
Copy link
Member Author

Will this be included in 0.5? :)

@benninkrs
Copy link

benninkrs commented Oct 6, 2016

I think workspace should do exactly what its name implies -- simply create a new workspace. This is a fundamental capability that probably needs to be retained. I think the ability to revert to the initial startup state, which is a more common and somewhat more sophisticated use case, should be handled by a distinct function whose name more clearly indicates this functionality, say, restart.

@tkelman tkelman added the needs tests Unit tests are required for this change label Oct 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants