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

If strict=true and a @repl block throws an exception, the build should fail #1447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DilumAluthge
Copy link
Contributor

Before this PR:

strict Block that throws an exception Build result
true @example ❌ fail
true @repl ✅ pass
false @example ✅ pass
false @repl ✅ pass

After this PR:

strict Block that throws an exception Build result
true @example ❌ fail
true @repl ❌ fail
false @example ✅ pass
false @repl ✅ pass

This is a breaking change.

@DilumAluthge
Copy link
Contributor Author

@mortenpi This PR takes care of the global behavior.

We still need the ability to opt-out on a per-block basis via the allowexceptions keyword. I'm not sure how to implement that, so I left it out of this PR. If you can assist me, we can put it in this PR. Or we can put it into a future PR.

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 think it would be good to do allowexceptions together with this change, so it would be easy for people who are relying on errors in at-repl to upgrade.

Re implementation: I think our current state of the art for keyword handling is this:

# "parse" keyword arguments to example (we only need to look for continued = true)
continued = occursin(r"continued\s*=\s*true", x.language)

I've always felt that we should have a function that takes the at-block lang. string and returns something that contains all the positional and keyword arguments. Maybe even relying on Meta.parse for the parsing? That said, a simple regex could also do. I think that's all we'd need?

src/Expanders.jl Outdated
Comment on lines 625 to 679
@info("""
encountered an exception while running a `@repl` block
that allows exceptions
""", exception=(c.value, c.backtrace))
Copy link
Member

Choose a reason for hiding this comment

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

If we're explicitly allowing exceptions, I don't think we want to write anything to the terminal? Could always turn it into an @debug though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah @debug sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to @debug

@mortenpi
Copy link
Member

I think our current state of the art for keyword handling is this

Actually, the state of the art is in doctests, so we should probably generalize that:

if startswith(lang, "jldoctest")
# Define new module or reuse an old one from this page if we have a named doctest.
name = match(r"jldoctest[ ]?(.*)$", split(lang, ';', limit = 2)[1])[1]
sym = isempty(name) ? gensym("doctest-") : Symbol("doctest-", name)
sandbox = get!(() -> Expanders.get_new_sandbox(sym), ctx.meta, sym)
# Normalise line endings.
block = MutableMD2CodeBlock(block_immutable)
block.code = replace(block.code, "\r\n" => "\n")
# parse keyword arguments to doctest
d = Dict()
idx = findfirst(c -> c == ';', lang)
if idx !== nothing
kwargs = Meta.parse("($(lang[nextind(lang, idx):end]),)")
for kwarg in kwargs.args
if !(isa(kwarg, Expr) && kwarg.head === :(=) && isa(kwarg.args[1], Symbol))
file = ctx.meta[:CurrentFile]
lines = Utilities.find_block_in_file(block.code, file)
@warn("""
invalid syntax for doctest keyword arguments in $(Utilities.locrepr(file, lines))
Use ```jldoctest name; key1 = value1, key2 = value2
```$(lang)
$(block.code)
```
""")
return false
end
d[kwarg.args[1]] = Core.eval(sandbox, kwarg.args[2])
end
end
ctx.meta[:LocalDocTestArguments] = d

@mortenpi
Copy link
Member

Just to check in with this: it just needs argument handling for the relevant at-blocks to locally set allowexceptions and it should be good to go I think?

@DilumAluthge
Copy link
Contributor Author

Just to check in with this: it just needs argument handling for the relevant at-blocks to locally set allowexceptions and it should be good to go I think?

Yep. I'll take a stab at implementing the keyword handling.

@mortenpi mortenpi modified the milestones: 0.26.0, 0.27.0 Dec 8, 2020
@DilumAluthge DilumAluthge force-pushed the dpa/at-repl-throw-exceptions branch from 0890821 to 9973808 Compare January 19, 2021 05:01
@DilumAluthge DilumAluthge force-pushed the dpa/at-repl-throw-exceptions branch from a892477 to 9760870 Compare March 17, 2021 20:04
@mortenpi mortenpi modified the milestones: 0.27.0, 0.28.0 Jun 10, 2021
@DilumAluthge DilumAluthge force-pushed the dpa/at-repl-throw-exceptions branch from 6c1ad26 to 6cb15db Compare December 24, 2021 06:54
@DilumAluthge
Copy link
Contributor Author

It's been a while since I looked at this.

@mortenpi Can you remind me where I should look to figure out how to handle the keyword arguments?

@mortenpi mortenpi removed this from the 0.28.0 milestone Jun 23, 2022
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.

2 participants