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

Assign PROGRAM_FILE earlier #22092

Merged
merged 7 commits into from
Jun 13, 2017
Merged

Assign PROGRAM_FILE earlier #22092

merged 7 commits into from
Jun 13, 2017

Conversation

omus
Copy link
Member

@omus omus commented May 26, 2017

I discovered a use case where it would be nice to be able to access PROGRAM_FILE while within the .juliarc.jl. The global variable is now assigned and available within --load file or the .juliarc.jl.

Additionally, PROGRAM_FILE is now a constant.

Follow up to #14114

I discovered a use case where it would be nice to be able to access
`PROGRAM_FILE` while within the .juliarc.jl. The global variable is
now assigned and available within `--load` file or the .juliarc.jl.

Additionally, PROGRAM_FILE is now a constant.
@omus
Copy link
Member Author

omus commented May 26, 2017

Note we may not want to back port this is a breaking change if users are accessing ARGS within their .juliarc.jl or within a --load file. Since Julia v0.6 isn't officially out yet I think this is ok.

@omus omus requested a review from tkelman May 26, 2017 23:36
@tkelman
Copy link
Contributor

tkelman commented May 26, 2017

I don't think it's good to change the behavior of juliarc this far after feature freeze, after 2 rc's.

Could you describe the use case more? Making juliarc behavior depend on PROGRAM_FILE seems like a strange thing to do

@omus
Copy link
Member Author

omus commented May 26, 2017

I needed to run Pkg.test without loading the ".juliarc.jl" under Julia 0.5. I'll make a separate PR for this later.

@omus
Copy link
Member Author

omus commented May 26, 2017

I don't think it's good to change the behavior of juliarc this far after feature freeze, after 2 rc's.

Completely reasonable. I'll drop the backport label.

@fredrikekre
Copy link
Member

I needed to run Pkg.test without loading the .juliarc.jl under Julia 0.5. I'll make a separate PR for this later.

Would this solve #21823 then?

@omus
Copy link
Member Author

omus commented May 27, 2017

Would this solve #21823 then?

This PR will not fix that issue but the other PR I'm working on will.

@omus
Copy link
Member Author

omus commented May 27, 2017

It may also be a good idea to set an ENV variable when running within the Pkg.build or Pkg.test subprocesses so that people write conditional code depending on the execution.

@omus
Copy link
Member Author

omus commented May 27, 2017

The separate PR I was referring to: #22094

@omus omus added the breaking This change will break code label May 27, 2017
[ci skip]
@omus
Copy link
Member Author

omus commented May 29, 2017

Added a NEWS entry since this a breaking change without a warning. Overall I think this change is still worthwhile as it makes PROGRAM_FILE a constant and makes the amount of ARGS consistent in all of the stages in which user code can be run.

@omus
Copy link
Member Author

omus commented Jun 6, 2017

Expanded PROGRAM_FILE tests to deal with startup-file. Code is now ready for a squash and merge.

println(PROGRAM_FILE)
include(\"$(escape(b))\")
"""
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, but I don't think it's usually worth using up vertical space to put close parens on their own line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update this to match the Base style

write(a, """
println(@__FILE__)
println(PROGRAM_FILE)
include(\"$(escape(b))\")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update this to use escape_string

@omus omus force-pushed the cv/program-file branch from 4b55c31 to c5a1506 Compare June 7, 2017 22:06
@omus
Copy link
Member Author

omus commented Jun 8, 2017

Ready to squash and merge

write(a, """
println(@__FILE__)
println(PROGRAM_FILE)
println(length(ARGS))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't care about testing ARGS length any more?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Originally I added this since the number of arguments could vary depending on when we would run the check. I could still include this check but it would only be for verifying that the ARGS were no longer includes the PROGRAM_FILE as ARGS[1]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not keep checking the new expected value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests prior to this ("test passing arguments") deals with testing the number of ARGS already. Having them in this section is redundant and just adds noise as the length(ARGS) == 0. I'll make sure that the previous section covers what these tests previously did.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new tests to address this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tony

@omus
Copy link
Member Author

omus commented Jun 12, 2017

Will merge later today

@omus
Copy link
Member Author

omus commented Jun 12, 2017

I'll hold off on merging today since I modified the tests. Essentially, the latest changes include:

  • Expanding the ARGS tests to include testing with the startup-file
  • Update the PROGRAM_FILE tests to always have the modified HOME
  • Make a helper function readsplit to make the PROGRAM_FILE tests easier to read

@omus omus merged commit 702256f into master Jun 13, 2017
@omus omus deleted the cv/program-file branch June 13, 2017 18:39
quinnj pushed a commit that referenced this pull request Jun 20, 2017
* Assign PROGRAM_FILE earlier

I discovered a use case where it would be nice to be able to access
`PROGRAM_FILE` while within the .juliarc.jl. The global variable is
now assigned and available within `--load` file or the .juliarc.jl.

Additionally, PROGRAM_FILE is now a constant.

* Add to NEWS

[ci skip]

* Test startup-file and PROGRAM_FILE

* Modify user home properly on Windows

* Style correction and remove escape function

* Expand arguments tests to include the startup-file

* Refactor PROGRAM_FILE tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants