-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 expression matching and use it in the doc system #12088
Conversation
That's definitely a lot clearer. So |
Yes, just for |
end | ||
end | ||
|
||
macro match (ex, lines) |
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.
erroneous space here (is deprecated).
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.
Which is a shame, because I like this style of writing macros (it matches the call better). I guess I'm not going to convince anyone else of that, though.
6496a4c
to
5380268
Compare
@@ -161,7 +161,7 @@ c,r,res = test_complete(s) | |||
|
|||
s = "\\a" | |||
c, r, res = test_complete(s) | |||
"\\alpha" in c | |||
("\\alpha" in c) |
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.
Is this needed?
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.
Previously the string was being interpreted as a docstring. Not sure what it was documenting but there you go.
Big -1 to merging this, I don't think this is needed in base. There's also been a registered Match.jl package for over 2 years (cc @kmsquire) that might not appreciate |
It's in "Need" is a relative term, I guess, but this does bring tangible improvements to the doc system that would be much harder to get right writing |
It's a net addition of quite a bit of code, so unless there are major restructurings pending that are not included here I don't see it as a simplification. It does look like a potentially useful set of functionality for writing macros, but not a net win to have it in base (which makes it effectively a mandatory dependency for all other code written in the language) at this time. The long-term trajectory should be for less code in base. Is this fixing bugs that cannot be fixed in any other way? |
In principle everything that's improved here / will be added to the doc system could be done the old way, sure. And you'd save around 100 LOC, optimising Base's code size by about 0.00084x – assuming this can't shorten code anywhere else in Base. Current bugs concern me less than potential future bugs as we add syntax to |
This is a bit like the argument about function chaining. People's aesthetic tastes differ - do whatever you want in your own code if no one will ever look at it, but in base code that everyone may need to touch, best to do things in the idiomatic way even if you personally would prefer a different style that you think looks nicer. "The old way" is the standard way macros are written everywhere else. There's a non-locality in this type of code where I have to understand and trust how I'm not going to fall on a sword or anything over this, would like to hear other opinions too. But how much more syntax is really going to need to be added to |
RE linked comment – after this PR, new doc syntax will no longer be breaking anyway, since anything that's not already recognised is an error. |
I had a go at cleaning up a couple other macros in Base (particularly (Incidentally, removing Enums would be a pretty easy way to save a hundred lines if that's what we're worried about. I'm not at all clear on what the logic is behind what's OK and what's not here.)
The thing is, I didn't put in all this effort just to make it look pretty. Clearly there's an aesthetic component here, and this PR has the pros and cons we've discussed on that front. But there's also an important practical upshot, which is that I, the maintainer of this code, can create and update it more quickly, easily and reliably. It will save me a lot of time and energy and the doc system will be better for it. If I thought perfect uniformity of style was more important than expressiveness I'd be writing Go. |
I'm not trying to be adamant about it, but I think what goes into base should receive more scrutiny as time goes on, the question should at least be asked. If Enums are a core part of the language, then they're naturally a dependency of other code (though since they're implemented "in user space" as it were, that may not always have to be the case). An implementation detail of the doc system that adds more confusing code than it removes, maybe not so much.
Once something goes into base, you're no longer the sole maintainer. Your particular code style has been a bit of an outlier, on previous occasions as well. Superficial stuff like whitespace will be fixed by an auto-formatter some day (Go absolutely got that part right). Less superficial style questions like this one should be discussed.
I'll say it again, write however you want in your own code if you're the only one who will ever read it. Adding something to base comes with relinquishing personal ownership to some extent, and accessibility and community norms should be a higher priority than personal taste. |
It's not fair to paint this as if it's all about personal ownership and forcing my own taste onto other people. I agree with you on all those points. To reiterate, what I'm attempting here – and it may or may not have been successful, but it is the intent – is to make maintaining the doc system easier for whomever is doing it (which happens to largely be me and @MichaelHatherly at the moment, though that could of course change – perhaps my choice of wording was poor earlier). To be completely clear: I do agree that if there's a consensus that this makes things less accessible, then it's the wrong thing to do. What I'm saying is that I think it does make things more accessible, as opposed to being the frivolous stylistic change you're making it out to be. And again, if there's a consensus that I'm wrong on that, fine – I'm not forcing my own taste on anyone. |
Sorry, I'm being overly curmudgeonly, which isn't helping my point. If we're going to do pattern-matching anywhere in base, let's have that discussion independent of its uses such as here in the doc system. And let others who've written similar code comment and contribute to the design. Code that uses pattern matching looks neat, but to me it comes across as a bit of a too-clever novelty syntax pun, and people who've never seen or used pattern matching before (which I'll postulate is the vast majority of Julia users) are going to have a really hard time understanding what's going on. |
Having a more general discussion and going over the implementation is definitely a good idea. I'll work on a new issue and try to put the case across a bit more thoroughly. |
What took a little while to click for me was the lack of quoting around each case. # ...
:(function f_(__) _ end) -> funcdoc(meta, def)
# ... instead of # ...
function f_(__) _ end -> funcdoc(meta, def)
# ... could possibly make things a little clearer and less magical, though slightly more verbose.
Definitely worth having something concrete to compare with/without pattern matching which I think this PR illustrates quite well. |
@MichaelHatherly FYI, I stripped the binding bits out of this to simplify it for review, but those changes are up on this branch if you want to play around with them – it was working really nicely when I used it and it'd probably make sense to move macros to using it too. |
Ah, thanks. This issue suddenly got really long and I must have got lost... :) Re: binding, looks to be the same approach I took with constants in Docile and didn't have any problems with it apart from slightly complicating doc retrieval. For macros to use that I presume you'd just store |
immutable T_ _ end -> typedoc(meta, def, namify(T)) | ||
(abstract T_) -> namedoc(meta, def, namify(T)) | ||
(bitstype _ T_) -> namedoc(meta, def, namify(T)) | ||
(typealias T_ _) -> objdoc(meta, def) |
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.
@one-more-minute, from this comment, can (typealias T_ _)
be rewritten as (typealias _ _)
?
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.
Yeah that would be fine
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.
Thanks, had a moment of doubt after answering @ssfrr's question.
Yeah you're exactly right about macros. I pushed a proof of concept for reference. |
This is a reasonably chunky addition to Base, but I think it's well worth it for the improvements it provides for the doc system macros – and I'm sure there are other areas where it would be useful as well.
It's now crystal clear what syntax is supported by the
@doc
macro, and what ends up doing what. I've also had a crack at doing #11850 – so between the two improvements, adding any new syntax we want should be very safe/easy from now on.Edit: Now have also implemented documentation for bindings themselves, as opposed to their values. We get a slicker way to get at macro expanders for free via
@var(@time)[]
.