-
Notifications
You must be signed in to change notification settings - Fork 501
Warn/error if jldoctest starts with an empty line; or lacks empty line between two REPL prompts #2808
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
base: master
Are you sure you want to change the base?
Conversation
f5b0bf0 to
4675d15
Compare
|
Advantages of a loose version:
|
4675d15 to
73dc44a
Compare
|
I am inclined to turn these into errors/warnings (i.e. doctest failures):
Requiring empty lines would mean that this case should be fine:
But it also shouldn't error. Also I feel like it should parse fine right now (but maybe it doesn't?). So maybe the correct thing to do here is to parse. And when a doctest fails, then check if there is a |
|
Thinking some more about this, my conclusion is that the argument "this code can be pasted fine into the REPL, so we should accept it" isn't so great after all. I think the converse which @mortenpi brought up at least implicitly carries more weight: what does the output of the REPL look like? I think having the rule "the doctest content should match the REPL output exactly is more important. Besides, if we reject it now, we can always switch to accepting it in the future. The other direction is much harder. So I'd say, let's indeed go with this PR over the other. I'll add some tests and a changelog entry. |
73dc44a to
04eb3ab
Compare
|
Add changelog entry, added a test -- I split this into two commits: the first commit adds the test with the current output. The second commit has the fix and adapts the test to the new output. This way one nicely sees the difference in quality between old and new message. |
cc2649b to
ab2f0e8
Compare
- Don't allow empty lines at start of jldoctest. - Require empty lines before `julia>` prompts other than the first one.
ab2f0e8 to
5ecf963
Compare

These are minor usability errors, "paper cuts" if you will, that have annoyed me repeatedly in the past, and clearly also other people:
Resolves #2031
Resolves #2083
Resolves #2679
Alternative to and hence closes #2810
Alternative in PR #2810: we could also just allow all of this. (For the leading empty line: Should it then be rendered, or hidden?)
Downside: such a doctest would not work with older Documenter versions. I therefore am inclined to prefer the strict version implemented here.
But in the end, either is preferable over the current errors.
(I am waiting with changelog and test cases until I know which direction we'll take)