-
Notifications
You must be signed in to change notification settings - Fork 120
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
Fix doctest_fix for Documenter 1.0 #2852
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one comment below you are free to ignore.
if isdefined(Main.Documenter, :DocTests) | ||
doctest = Main.Documenter.DocTests.doctest | ||
else | ||
doctest = Main.Documenter._doctest | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isdefined(Main.Documenter, :DocTests) | |
doctest = Main.Documenter.DocTests.doctest | |
else | |
doctest = Main.Documenter._doctest | |
end | |
if isdefined(Main.Documenter, :_doctest) | |
doctest = Main.Documenter._doctest | |
else | |
doctest = Main.Documenter.DocTests.doctest | |
end |
I would be more inclined to switch up the order, such that the Documenter 1.0 version is the first one. This makes the future dropping of 0.27 support clearer and more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree that it is clearer, one still has to know which version is what. If we want this to be "clear" we need to switch this and other existing checks to check for a version. But that seems to be investing 5 minutes of work now to save 5 minutes of work later, so IMHO not worth it :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True :)
Codecov Report
@@ Coverage Diff @@
## master #2852 +/- ##
==========================================
- Coverage 80.67% 80.65% -0.02%
==========================================
Files 456 456
Lines 64717 64720 +3
==========================================
- Hits 52208 52203 -5
- Misses 12509 12517 +8
|
No description provided.