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

Do not enforce type Symbol for argument mod #208

Closed
wants to merge 3 commits into from

Conversation

devmotion
Copy link
Contributor

Hej!

I noticed that as soon as options are specified in the YAML header of a file, it is not possible anymore to weave in a module such as Main. The problem is that during parsing and merging of the header arguments mod is converted to a Symbol in https://github.com/mpastell/Weave.jl/blob/master/src/config.jl#L112, but https://github.com/mpastell/Weave.jl/blob/master/src/run.jl#L57 requires mod to be a Module.

As an example, for a simple file test.jmd such as

---
options:
  out_path: test.md
  doctype: github
---

This is a test.

the command

weave("test.jmd", mod = Main)

fails with the error message

ERROR: MethodError: no method matching eval(::Symbol, ::Expr)
Closest candidates are:
  eval(::Module, ::Any) at boot.jl:328
Stacktrace:
 [1] #run#29(::String, ::Symbol, ::String, ::Dict{Any,Any}, ::String, ::Nothing, ::String, ::Symbol, ::Bool, ::typeof(run), ::Weave.WeaveDoc) at /home/david/.julia/packages/Weave/L0NNS/src/run.jl:57
 [2] (::getfield(Base, Symbol("#kw##run")))(::NamedTuple{(:doctype, :mod, :out_path, :args, :fig_path, :fig_ext, :cache_path, :cache, :throw_errors),Tuple{String,Symbol,String,Dict{Any,Any},String,Nothing,String,Symbol,Bool}}, ::typeof(run), ::Weave.WeaveDoc) at ./none:0
 [3] #weave#16(::Symbol, ::Symbol, ::Symbol, ::Dict{Any,Any}, ::Module, ::String, ::Nothing, ::String, ::Symbol, ::Bool, ::Nothing, ::Nothing, ::Nothing, ::Array{String,1}, ::String, ::typeof(weave), ::String) at /home/david/.julia/packages/Weave/L0NNS/src/Weave.jl:121
 [4] (::getfield(Weave, Symbol("#kw##weave")))(::NamedTuple{(:mod,),Tuple{Module}}, ::typeof(weave), ::String) at ./none:0
 [5] top-level scope at none:0

This PR fixes that issue (and replaces getvalue with the equivalent get) - however, I'm wondering whether a mod option in the header should be handled differently. It seems "sandbox" is the only valid option since everything else will fail due to the same issue? Maybe one should explicitly check if it is set to "Main"?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 81.877% when pulling d5d954f on devmotion:mod into c9a8147 on mpastell:master.

@mpastell
Copy link
Collaborator

Thanks! Probably it would be good to check whether it is set to sandbox as that is the only valid "non-module" option. It is possible to use other modules that Main. The only possibly useful situation I can think of is to use the same sandbox module for several documents.

Could you make the change?

@devmotion
Copy link
Contributor Author

devmotion commented Jun 28, 2019

With the latest commit the following behaviour is enforced:

  • For the file test.jmd

    ---
    options:
      out_path: test.md
      doctype: github
      mod: sandbox
    ---
    
    This is a test.
    

    the module specified on the command line is used, i.e.,

    weave("test.jmd")

    and

    weave("test.jmd", mod = :sandbox)

    run in a sandbox module whereas e.g.

    weave("test.jmd", mod = Main)

    uses the Main module.

  • For the file

    ---
    options:
      out_path: test.md
      doctype: github
      mod: sandbox
    ---
    
    This is a test.
    

    a sandbox module is used, regardless of the setting of mod on the command line.

  • For the file

    ---
    options:
      out_path: test.md
      doctype: github
      mod: sandbox2
    ---
    
    This is a test.
    

    a warning is displayed, the header option is ignored, and the module that is specified on the command line is used.

src/config.jl Outdated
if m == "sandbox"
mod = :sandbox
else
@warn "The only valid header option for 'mod' is 'sandbox'. Ignoring value '$m'."
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to set mod = Module(Symbol(m)) here. I don't think there is any reason not to be able to specify module name in header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, I misunderstood your previous comment. I assumed it should be restricted to ":sandbox" only.

In that case I think it's more reasonable to handle all mod options of type Symbol in the run method, such that weave("test.jmd", mod = :sandbox3) for a file test.jmd without header arguments and weave("test_headers.jmd") for a file test_headers.jmd with header arguments mod: sandbox3 behave the same.

@aviatesk aviatesk closed this May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants