Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement type-based API for progress records #13
Implement type-based API for progress records #13
Changes from all commits
d1e1b0e
016b168
38b6ca9
3231b1c
1411953
f72c270
1018ae0
5cad552
64aa398
5d938fd
993eb6d
247b154
27665aa
066ec03
ace920e
2b13ad0
d4a726f
8217339
8c135cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Clever! Would the plan be to remove this and just use bare
Progress
in the future?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 think @pfitzseb wants to keep the original open/untyped API. I'm OK for keeping this as long as this compatible layer becomes annoying hard to maintain.
ProgressString
is somewhat already "too clever" but maybe it's within an OK range.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'm not sure I followed everything which was going on with interpolation and macro hygiene here!
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 think I had a trouble when using a macro inside a macro. I tend to give up fighting hygiene and
esc
ape the whole block when it happens. I'll see if I can avoid this after writing tests.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 you can make it neater by all means that would be great.
But I agree that sometimes manual escaping is just easier 😞
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.
So, I tried to avoid interpolation but it didn't work. Here is a MWE:
We could use
Expr(:isdefined, var)
(orExpr(:islocal, var)
) directly likeBase
is doing:JuliaLang/julia@4800158#diff-121162110854f09c3be5b39209646009L353
But I prefer sticking with the high-level API
@isdefined
sinceExpr(:isdefined, :x)
is listed only at lowered AST section: https://docs.julialang.org/en/latest/devdocs/ast/#Lowered-form-1There 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.
Yes the special forms without surface syntax are implementation details of
Base
so best to avoid them.I expect the error here is because nested macros see the
Expr(:esc, ...)
from the parent macro and@isdefined
can't deal with that. It's an unsolved problem (JuliaLang/julia#23221 and all that).