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

Add option to change working directory for evaluation of example code #1025

Merged
merged 22 commits into from
Jun 13, 2019

Conversation

kescobo
Copy link
Contributor

@kescobo kescobo commented May 21, 2019

Attempt to address #1013

  • Add working_dir option to makedocs
  • Successfully build test site with evaluation in non-build directory
  • Add tests
  • Add documentation

I've added a working_dir option to makedocs(), which is intended to allow a user to set where code in @example and @repl blocks is evaluated from. The default value is :build which recapitulates current behavior.

@kescobo
Copy link
Contributor Author

kescobo commented May 21, 2019

I'm testing on a document called test.md:

# Testing

Here's a test

```@repl
pwd()
```

Default behavior works as expected, but if I pass strings like "src", "build" or "test", I get the same output for all of them:

julia> pwd()
"/Users/ksb/repos/julia_pkgs/DocumenterDev/docs"

But if I pass "." I get

julia> pwd()
ERROR: IOError: chdir docs: no such file or directory (ENOENT)

or if I pass ".." I get

julia> pwd()
ERROR: IOError: chdir DocumenterDev/docs: no such file or directory (ENOENT)

Clearly I'm missing something about how the paths are passed through - any help would be appreciated. Also happy to take other comments / suggestions of course

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

For testing, an option would be to set up a simple site build similar to test/examples/, say under test/workdir/.

src/Documents.jl Outdated Show resolved Hide resolved
src/Documents.jl Outdated Show resolved Hide resolved
src/Expanders.jl Outdated Show resolved Hide resolved
src/Documents.jl Outdated Show resolved Hide resolved
@mortenpi mortenpi added this to the 0.23.0 milestone May 22, 2019
@kescobo
Copy link
Contributor Author

kescobo commented May 23, 2019

So, this now works locally. If I do julia --project=@. --color=yes test/workdir/make.jl, it correctly makes workdir.txt in the correct folder.

Other tests all pass, but this is causing an error outside of tests:

Documenter: Error During Test at /Users/ksb/.julia/dev/Documenter/test/runtests.jl:18
  Got exception outside of a @test
  LoadError: IOError: open: no such file or directory (ENOENT)

I copied the code from nongit, but I'm not actually clear on what the test is in nongit. Is it just looking for a lack of error?

@kescobo
Copy link
Contributor Author

kescobo commented May 23, 2019

FWIW, other than #1027, this branch now works for my use-case 🎉

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

For docs, it should have an entry in the makedocs' docstring. And would you also mind adding an entry to CHANGELOG.md?

src/Builder.jl Outdated Show resolved Hide resolved
src/Builder.jl Outdated Show resolved Hide resolved
@mortenpi
Copy link
Member

I took the liberty of re-organizing the tests a bit. I marked a few as broken though until it's clear how we want to handle relative paths.

@kescobo
Copy link
Contributor Author

kescobo commented May 31, 2019

That's awesome, thanks! I like your changes, and all of your tests now pass. How should I bring this up to date with master - do you have preferences about rebase vs merge commits?

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. LGTM other than the small buildnode thing.

How should I bring this up to date with master - do you have preferences about rebase vs merge commits?

I would probably just squash merge this, if that sounds reasonable to you.

src/Documenter.jl Outdated Show resolved Hide resolved
src/Documents.jl Outdated Show resolved Hide resolved
@kescobo
Copy link
Contributor Author

kescobo commented Jun 5, 2019

I would probably just squash merge this, if that sounds reasonable to you.

That's fine with me! Please let me know if there's anything else you need, and thanks for helping me through this :-)

@kescobo kescobo changed the title [WIP] Add option to change working directory for evaluation of example code Add option to change working directory for evaluation of example code Jun 5, 2019
@kescobo
Copy link
Contributor Author

kescobo commented Jun 6, 2019

The appveyor failure only occurs on julia 1.1 x64 and seems unrelated to this PR? I'm showing Travis as pending, but when I click through to the details it looks like everything is done and passing. @mortenpi Anything else to do here? (PS - I saw that you ended up adding this to CHANGLOG.md, thanks for that!)

@fredrikekre
Copy link
Member

fredrikekre commented Jun 7, 2019

I appreciate the work you have put into this PR. However, it feels a bit wrong with a global option for this. Would it be better with something in an @meta -block? E.g.

```@meta
WorkingDirectory = "..."
```

?

Also, regardless of this we should store the pwd() and re-use the one for blocks with the same name. That would make this pattern work: #1013 (comment)

@mortenpi
Copy link
Member

mortenpi commented Jun 7, 2019

However, it feels a bit wrong with a global option for this. Would it be better with something in an @meta -block?

I do feel that per-file option would be better in principle. So I am thinking that it might be a good idea to declare workdir an experimental keyword. That way @kescobo, you could use it for your use case, but we could also easily replace it when something better comes along.

@mortenpi
Copy link
Member

mortenpi commented Jun 12, 2019

I marked it experimental so that we could go ahead and merge this, but also potentially revisit these decisions in the future.

As a side note, we should probably move some of the other "experimental" keywords out the experimental section -- e.g. strict = true is not really experimental anymore I think.

Edit: test failures unrelated, see #1037.

@mortenpi mortenpi merged commit 3e77205 into JuliaDocs:master Jun 13, 2019
@mortenpi
Copy link
Member

I hope I didn't find one these compromises that makes everyone unhappy 😆

Thank you @kescobo for your work on this! Are you fine with using Documenter on master for a while? I think it might be a few weeks before we do another (minor) release.

@kescobo
Copy link
Contributor Author

kescobo commented Jun 14, 2019

@mortenpi This is great! Thanks so much for shepherding me through this :-). Definitely happy to be on Documenter master for this project while waiting for a merge 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants