add doctest = :fix option to makedocs #656

fredrikekre · 2018-03-05T12:53:32Z

This makes it easier to fix multiple outdated doctests in one command, which is useful to do e.g. when preparing a new release.

Not really sure how to test this.

fix #448

fredrikekre · 2018-03-05T15:27:05Z

Will do something for #448 (comment) too.

fredrikekre · 2018-03-05T16:04:11Z

Added a commit that looks for the complete code-snippet before doing the replacement instead of just searching for the result. This should eliminate the risk of double replacement as described in #448 (comment) in 99.9% of cases.

fredrikekre · 2018-03-05T16:39:56Z

Actually, this will still turn

```jldoctest
julia> 1 + 1
3

julia> 1 + 2
3
```

into

```jldoctest
julia> 1 + 1
2

julia> 1 + 2
2
```

fredrikekre · 2018-03-05T22:58:01Z

Okay, this version should work in all (?) cases:
First search in the file for the whole code snippet (rather than just the output string), then search within the code block for the input string, and last replace the old output with the new one.

Will add some tests, but feel free to review the implementation.

fredrikekre · 2018-03-06T07:15:55Z

Turns out the method I described in the last comment still failed in for the following case:

```jldoctest
julia> 1 + 2
2

julia> 1 + 2
2
```

because when we try to fix the second fail in this block we can not find that block in the file, since we replaced it with

```jldoctest
julia> 1 + 2
3

julia> 1 + 2
2
```

I solved this by storing the full Markdown.Code object in Result and then when replacing the content in the file we also replace the content in the Markdown.Code object such that the internal representation mathces that in the file.

I have now also added tests with the corner cases I have found when working on this.

fredrikekre · 2018-03-06T07:17:35Z

src/DocChecks.jl

@@ -343,8 +353,8 @@ function report(result::Result, str, doc::Documents.Document)
 println(ioc)
 printstyled(ioc, "> File: ", result.file, "\n", color=:cyan)
 printstyled(ioc, "\n> Code block:\n", color=:cyan)
- println(ioc, "\n```jldoctest")
- println(ioc, result.code)
+ println(ioc, "\n```$(result.block.language)")


Storing the full Markdown.Code object also allows to print the actual language field here, including name, filter and setup. Previously it was a bit confusing that those things were left out from the error printing.

mortenpi

This is neat, thanks for this!

There's an edge case it can't handle at the moment (it fixes the wrong doctest):

```@meta
DocTestSetup = quote
    foo() = 42
end
```

```jldoctest
julia> foo()
42
```

```@meta
DocTestSetup = quote
    foo() = 43
end
```

```jldoctest
julia> foo()
42
```

mortenpi · 2018-03-06T06:46:57Z

docs/src/man/doctests.md

+
+To fix outdated doctests, the `doctest` flag to [`makedocs`](@ref) can be set to
+`doctest = :fixup`. This will run the doctests, and overwrite the old results.
+


Might be good to recommend here that the user should git commit their code changes, just in case, before running the doc build with this option on. That way, should the find-and-replace algorithm go a bit wonky and completely mess up the source file, the user can easily just git reset --hard to their state.

mortenpi · 2018-03-06T07:18:37Z

docs/src/man/doctests.md

+To fix outdated doctests, the `doctest` flag to [`makedocs`](@ref) can be set to
+`doctest = :fixup`. This will run the doctests, and overwrite the old results with
+the new output.
+


Might be good to recommend here that the user should git commit their code changes, just in case, before running the doc build with this option on. That way, should the find-and-replace algorithm go a bit wonky and completely mess up the source file, the user can easily just git reset --hard to their state.

mortenpi · 2018-03-06T07:25:48Z

test/doctests.jl

+ str = read(f, String)
+ @test str == good_str
+ end
+end


This should fix doctests in .jl files too, right? Could perhaps also test that, to make sure that the file paths get resolved correctly etc.

fredrikekre · 2018-03-06T07:34:30Z

Yea thats a corner case I also thought of. But it should be pretty rare(?) that the same input yields different output. Will add the recommendation to git commit and also recommend that the result should be inspected manually.

mortenpi · 2018-03-06T07:40:29Z

But it should be pretty rare(?) that the same input yields different output.

Agreed. As you said, a note in the docs saying that there are potential edge cases etc. should be enough.

KristofferC · 2018-03-06T10:51:27Z

Great stuff to have!

fredrikekre · 2018-03-06T11:01:24Z

Hmm, this will not actually work for script-doctests....

fredrikekre · 2018-03-06T11:13:58Z

Ok, we are good I think.

fredrikekre · 2018-03-06T11:17:33Z

src/DocChecks.jl

 # TODO: decide whether to keep `# output` syntax for this. It's a bit ugly.
 # Maybe use double blank lines, i.e.
 #
 #
 # to mark `input`/`output` separation.
- input, output = split(code, "\n# output\n", limit = 2)
- result = Result(code, "", output, meta[:CurrentFile])
+ input, output = split(block.code, "\n# output\n\n", limit = 2)


Any dangers with this? The problem was that output here became e.g. "\n2" while the result string became "3", thus we replaced

```jldoctest 1 + 2 # output 2 ```

with

```jldoctest 1 + 2 # output 3 ```

instead of

```jldoctest 1 + 2 # output 3 ```

Perhaps it would be safer to do

input, output = split(block.code, "# output\n", limit = 2) input = rstrip(input, '\n') output = lstrip(output, '\n')

?

From the manual:

Note that the amount of whitespace appearing above and below the # output line is not significant and can be increased or decreased if desired.

So we should strip '\n' instead

We should also support Windows line endings here I think, which would not get stripped correctly at the moment, right? This might work though?

input = rstrip(input, ['\n', '\r']) output = lstrip(output, ['\n', '\r'])

fredrikekre · 2018-03-07T09:18:52Z

Perhaps we should change :fixup -> :fix. I like doctest = (true|false|:fix) better I think. Thoughts?

mortenpi · 2018-03-08T00:08:53Z

Yup, :fix looks better.

fredrikekre · 2018-03-08T07:35:13Z

doctest = (:yes|:no|:fix) might be even better, but w/e. Changed :fixup -> fix.

mortenpi · 2018-03-09T22:12:24Z

src/DocChecks.jl

+ truncate(f, 0)
+ # first look for the entire code block
+ codeidx = findnext(code, content, 1)
+ @assert codeidx != 0:-1 # this should never happen


If you have CRLF line endings then this throws. It also leaves the source file empty on error right now.. so even if we fix this particular case, I think it would probably be a good idea to try ... catch all of this and restore the original file when are any errors (and then rethrow).

Good catch. We could potentially try to detect the line ending style, unless there could sometimes be a mix of the two?
Probably good to wrap in try catch anyway

fredrikekre · 2018-03-10T19:18:22Z

Pushed a new version that should fail more gracefully if we don't find the code block. Also leaves the source file untouched in case of failure.

mortenpi · 2018-03-12T06:44:01Z

src/DocChecks.jl

+ return
+ end
+ # write everything up until the code block
+ write(io, content[1:prevind(content, first(codeidx))])


I was able to trigger a

ERROR: LoadError: BoundsError: attempt to access "# Test\r\n\r\n```jldoctest\r\njulia> 2 + 2\r\n0000\r\n```\r\n" at index [0] Stacktrace: [1] prevind(::String, ::Int64, ::Int64) at ./strings/basic.jl:426 [2] prevind at ./strings/basic.jl:421 [inlined] [3] fix_doctest(::Documenter.DocChecks.Result, ::SubString{String}, ::Documenter.Documents.Document) at /home/morten/.julia/v0.7/Documenter/src/DocChecks.jl:395 [4] checkresult(::Module, ::Documenter.DocChecks.Result, ::Dict{Symbol,Any}, ::Documenter.Documents.Document) at /home/morten/.julia/v0.7/Documenter/src/DocChecks.jl:305

with CRLFs (this file) right now. So this manages to avoid your checks. Sorry for keeping on breaking this 😆

Hmm.. How? I simply get:

!! Could not find code block in source file

Could it be that you don't have JuliaLang/Compat.jl@5ff9892 ?

Ah, I think you're right, my bad. I happened to be running this with my 0.7 binary and I think it is a bit too old (9 days).

mortenpi · 2018-03-12T06:47:45Z

src/DocChecks.jl

+ codeidx = Compat.findnext(code, content, 1)
+ if codeidx === nothing
+ Utilities.warn("Could not find code block in source file")
+ return


This check (and the one below) should still never happen, right? Perhaps we should keep throwing/erroring/asserting here so that in case they are triggered, they don't go unnoticed.

My thought here was that since we have replaces \r\n with \n in the code block

Documenter.jl/src/DocChecks.jl

Lines 133 to 134 in b64a4d7

# Normalise line endings.

block.code = replace(block.code, "\r\n" => "\n")

we would obtain nothing here since we have not performed the replacement in the file where we search.

mortenpi · 2018-03-12T06:51:21Z

test/doctests.jl

+\"\"\"
+foo() = 1
+end # module
+"""


Having a test with CRLFs wouldn't hurt. We could also keep these in separate files and just cp them over to tempdir to avoid problems with differing line endings in a single file (although a simple replace(..., '\n' => "\r\n") works as well I guess).

fredrikekre · 2018-03-12T08:49:30Z

So currently this will just do nothing for CRLF files. It is trivial to support this, by simply normalizing the lineendings when reading the source file before we search for the code block. The problem though is that when we print it back to the file we have updated all line endings. We could do this, or somehow try to detect which linendings are used and replace with that before printing back to file. That will however be hard if a file has a combination of lineendings.

mortenpi · 2018-03-12T08:57:49Z

I don't think it should be changing line endings of unrelated lines. But we would only need to figure out the line ending for each doctest separately, right? Could we, say, count the LFs and CRLFs to determine which one it is and then normalize to that? I think we could live with changing a mixed doctest to either pure LF or CRLF.

In any case though, I'd be happy to merge this with just LF fixing as well, and it can be improved in the future if there's demand. It will already be massively useful for Base which is LF. Should just probably have a note in the manual about CRLFs.

fredrikekre · 2018-03-12T10:27:53Z

It is a bit tricky since the byte index in the string will change after we have replaced the line endings. I added a note that this currently only works for LF line endings for now.

Also added some better tests, including multiline output which was missing before.

fredrikekre · 2018-03-12T18:21:50Z

I have tried this with Julia base and some other projects and it works well for real projects too, not just the trivial test cases in the tests, so if you are happy with the current state, feel free to merge and we can work out possible problems as we find them.

fredrikekre · 2018-03-12T18:27:37Z

I should just mention that it does not quite work together with doctest filters, but perhaps easier to review the changes to make that work in a separate PR.

mortenpi · 2018-03-13T07:31:58Z

Yup, let's iterate on this in future PRs, if the need arises. Thanks a lot for this, this is a really neat feature!

fredrikekre · 2018-04-23T14:10:17Z

This is a pretty useful feature when updating packages, doing this Ferrite-FEM/Tensors.jl@213ac2c manually would take some time.

fredrikekre added the Type: Feature label Mar 5, 2018

fredrikekre requested review from mortenpi and KristofferC March 5, 2018 12:53

fredrikekre force-pushed the fe/doctest-fix branch from 4f36e14 to 3d44803 Compare March 5, 2018 12:54

fredrikekre force-pushed the fe/doctest-fix branch from 2b0699d to eb801f7 Compare March 5, 2018 16:12

fredrikekre force-pushed the fe/doctest-fix branch from eb801f7 to 75981e0 Compare March 5, 2018 22:55

fredrikekre force-pushed the fe/doctest-fix branch from 75981e0 to e9b466f Compare March 6, 2018 07:11

fredrikekre commented Mar 6, 2018

View reviewed changes

fredrikekre force-pushed the fe/doctest-fix branch from e9b466f to c8ddea6 Compare March 6, 2018 07:23

mortenpi reviewed Mar 6, 2018

View reviewed changes

mortenpi added this to the 0.15.0 milestone Mar 6, 2018

fredrikekre force-pushed the fe/doctest-fix branch 2 times, most recently from 43d5e19 to d76fcd9 Compare March 6, 2018 08:45

fredrikekre force-pushed the fe/doctest-fix branch from d76fcd9 to 367ab39 Compare March 6, 2018 11:13

fredrikekre commented Mar 6, 2018

View reviewed changes

fredrikekre force-pushed the fe/doctest-fix branch from 367ab39 to 1ca793b Compare March 6, 2018 11:28

fredrikekre force-pushed the fe/doctest-fix branch from 1ca793b to 654b2fa Compare March 8, 2018 07:32

fredrikekre changed the title ~~add doctest = :fixup option to makedocs~~ add doctest = :fix option to makedocs Mar 8, 2018

fredrikekre force-pushed the fe/doctest-fix branch from 654b2fa to 5413f32 Compare March 8, 2018 08:41

mortenpi reviewed Mar 9, 2018

View reviewed changes

fredrikekre force-pushed the fe/doctest-fix branch from 881b759 to ab6d199 Compare March 10, 2018 19:17

fredrikekre force-pushed the fe/doctest-fix branch 3 times, most recently from 8a14599 to b64a4d7 Compare March 11, 2018 16:06

mortenpi reviewed Mar 12, 2018

View reviewed changes

fredrikekre force-pushed the fe/doctest-fix branch from b64a4d7 to 6542833 Compare March 12, 2018 10:25

add doctest = :fix option to makedocs

03e0a58

fredrikekre force-pushed the fe/doctest-fix branch from 6542833 to 03e0a58 Compare March 12, 2018 11:09

mortenpi merged commit 709b7c7 into master Mar 13, 2018

mortenpi deleted the fe/doctest-fix branch March 13, 2018 07:32

This was referenced Mar 13, 2018

doctest = :fix with filters #670

Merged

Upgrade Documenter, and add make target to fix doctests JuliaLang/julia#26515

Merged


		To fix outdated doctests, the `doctest` flag to [`makedocs`](@ref) can be set to
		`doctest = :fixup`. This will run the doctests, and overwrite the old results.

	# Normalise line endings.
	block.code = replace(block.code, "\r\n" => "\n")

add doctest = :fix option to makedocs #656

add doctest = :fix option to makedocs #656

Conversation

fredrikekre commented Mar 5, 2018 • edited Loading

fredrikekre commented Mar 5, 2018

fredrikekre commented Mar 5, 2018 • edited Loading

fredrikekre commented Mar 5, 2018

fredrikekre commented Mar 5, 2018

fredrikekre commented Mar 6, 2018

Choose a reason for hiding this comment

mortenpi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortenpi Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

fredrikekre commented Mar 6, 2018 • edited Loading

mortenpi commented Mar 6, 2018

KristofferC commented Mar 6, 2018

fredrikekre commented Mar 6, 2018

fredrikekre commented Mar 6, 2018

fredrikekre Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre commented Mar 7, 2018

mortenpi commented Mar 8, 2018

fredrikekre commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre commented Mar 12, 2018

mortenpi commented Mar 12, 2018

fredrikekre commented Mar 12, 2018

fredrikekre commented Mar 12, 2018

fredrikekre commented Mar 12, 2018

mortenpi commented Mar 13, 2018

fredrikekre commented Apr 23, 2018

fredrikekre commented Mar 5, 2018 •

edited

Loading

fredrikekre commented Mar 5, 2018 •

edited

Loading

mortenpi left a comment •

edited

Loading

mortenpi Mar 6, 2018 •

edited

Loading

fredrikekre commented Mar 6, 2018 •

edited

Loading

fredrikekre Mar 6, 2018 •

edited

Loading