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 a prefix when replacing for doctest=:fix #2378

Merged
merged 27 commits into from
May 1, 2024

Conversation

lkastner
Copy link
Contributor

Attempt fixing #2303
The logic is as follows: The repl session is split into two parts, the first part goes from the start until the current command, and the second part is the rest. The old output is removed from the second part and replaced by the correct output. However if the command e.g. was just a for displaying the variable a, then the split could incorrectly happen at the a in julia>. The new logic introduces a prefix containing the entire repl until the current command, so the split happens after (prefix + command) is found.

@mortenpi
Copy link
Member

Sorry for the delay here. I can confirm that the bug is there, but it doesn't look like the PR fixes it right now. I'm getting this after fixing:

```jldoctest
julia> a
("a", "b", "c") = ("a", "b", "c");

julia> a

```

Which is slightly different than the original bug, but still wrong.

cc @fredrikekre: if you have any bandwith to help with this?

@lkastner
Copy link
Contributor Author

Sorry for the delay here. I can confirm that the bug is there, but it doesn't look like the PR fixes it right now. I'm getting this after fixing:

```jldoctest
julia> a
("a", "b", "c") = ("a", "b", "c");

julia> a

This should work now. The testcases I added never had the output suppressed with ; in the first command which kept the prefix from initializing. And unfortunately I made up the example for the PR test without running it. Murphy. I am sorry about that.

@lkastner
Copy link
Contributor Author

There are some other cleaner approaches but these would require a larger refactoring, so I just went for a minimal approach.

@lkastner
Copy link
Contributor Author

@benlorenz pointed out to me, that the tests are failing now because Julia has changed the output format for errors. Here is the difference:
First 1.6:

julia> a
ERROR: UndefVarError: a not defined

Now current Julia:

julia> a
ERROR: UndefVarError: `a` not defined

i.e. the backticks were added. I added a commit to account for this in the tests by having a separate file for 1.6.

Copy link
Contributor

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

This looks fine to me (I made two trivial and optional change suggestions to match the style of the surrounding code).

It would be a pity if this fix for a real-world problem would be wasted :-(

src/doctests.jl Outdated Show resolved Hide resolved
src/doctests.jl Outdated Show resolved Hide resolved
mortenpi and others added 2 commits March 26, 2024 10:45
@mortenpi mortenpi linked an issue Mar 25, 2024 that may be closed by this pull request
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.

Thank you @lkastner! Apologies that it took a while to get this reviewd -- this is a great PR! We'll have it in 1.4.0, and I think we can tag that this week.

I took the liberty of adding a CHANGELOG and also editing the code a bit, to simplify it a bit and make it a bit more Julia (in my opinion).

@mortenpi mortenpi enabled auto-merge (squash) March 26, 2024 00:34
@mortenpi mortenpi disabled auto-merge March 26, 2024 00:34
@mortenpi
Copy link
Member

It looks like there's still a regression, or some issue, here. This doctest

```jldoctest
julia> a
ERROR: UndefVarError: `a` not defined

julia> b
```

does not fix itself on nightly (and causes the test failures). But it (more or less) works with the latest Documenter. I say more or less, since it will get replaced with

```jldoctest
julia> a
ERROR: UndefVarError: `a` not defined

julia> b
ERROR: UndefVarError: `b` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
```

so the first doctest is not being fixed for some reason.

@mortenpi mortenpi mentioned this pull request Apr 14, 2024
6 tasks
Previously it was enough to check for substring also in the doctest=:fix
mode, but this does not work with more complicated error messages
@fingolfin
Copy link
Contributor

Could someone with the relevant permissions (e.g. @mortenpi) please allow the CI tests to run?

@lkastner
Copy link
Contributor Author

I managed to fix the tests on nightly. The main reason for failure was that the error messages in nightly have an

in `Main`

added. I modified the error messages in the tests and have the test.jl remove these parts for older versions of Julia.

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.

I'm happy to merge this. Thank you very much for sticking with this, and sorry it took so long to get this done and merged!

@mortenpi mortenpi enabled auto-merge (squash) May 1, 2024 00:38
@mortenpi mortenpi merged commit 49f80af into JuliaDocs:master May 1, 2024
22 checks passed
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.

doctest(Module; fix=true) produces weird results
3 participants