Uh oh!
There was an error while loading. Please reload this page.
Document custom derive. - #38770
Conversation
rust-highfive
commented
Jan 2, 2017
@steveklabnik: no appropriate reviewer found, use r? to override |
steveklabnik
commented
Jan 2, 2017
/cc @rust-lang/docs @rust-lang/lang |
Would it make sense to document it in the reference as well? Derive is documented there too. |
steveklabnik
commented
Jan 2, 2017
Ah ha! I knew I forgot something. Yes. |
killercup
left a comment
There was a problem hiding this comment.
Nice. Added a few inline comments with suggestions :)
There was a problem hiding this comment.
Shouldn't that be src/main.rs?
There was a problem hiding this comment.
Maybe add a sentence about naming convention? I've seen a bunch of crates that call these <name>_derive.
There was a problem hiding this comment.
Link "syn" and "quote" to their repositories or crates.io pages?
There was a problem hiding this comment.
People who don't know nom will be confused by this.
There was a problem hiding this comment.
Lowercase "rust", same a few times below
There was a problem hiding this comment.
Double space after sentence here and a few times below; I don't think the rest of the book does this.
There was a problem hiding this comment.
Hm, maybe "serializing rust code" → "generating Rust code"?
There was a problem hiding this comment.
Error handling probably deserves a short paragraph as well, even if it's just "panicking the proc macro code will show it as a compiler error pointing to the derive".
withoutboats
commented
Jan 2, 2017
How do you feel about making |
steveklabnik
commented
Jan 2, 2017
Okay! I think I've fixed up everything, @withoutboats@killercup . Thanks for the review 😄 Still have to update the reference, should push that in a minute. |
There was a problem hiding this comment.
FYI this path uses backslashes, the rest of the book probably not
There was a problem hiding this comment.
It's a little weird to me that ## Macros is a subheading within # Macros, but I see why you did it
There was a problem hiding this comment.
yeah i would like to re-do this and other parts of the reference but i'm trying to be minimally invasive here
There was a problem hiding this comment.
Earlier in this section, "derive" is styled as
`derive`
with backticks, so might be good to be consistent.
There was a problem hiding this comment.
s/t will/it will/
It looks like crate types are styled with backticks, so also:
s/proc-macro/`proc-macro`/
nikomatsakis
left a comment
There was a problem hiding this comment.
This seemed like a decent summary. The one thing I would say is that I would have expected a bit more up-front material discussing the 'basic model' for derive (string in, string out, you do some parsing and serialization in the model), but that's because I tend towards more of a "top-down" style, I think, whereas this is kind of "bottom-up". (i.e., here is the code to write, let's explain why you wrote it) In any case, all the important stuff seems to be there, and it reads pretty smoothly.
I left some nits.
There was a problem hiding this comment.
Nit: s/repition/repetition/
There was a problem hiding this comment.
Nit: s/Procedrual/Procedural/
There was a problem hiding this comment.
Nit: s/Procedrual/Procedural/
There was a problem hiding this comment.
i am... really bad at this spelling apparently, haha
There was a problem hiding this comment.
This stream of consciousness bit felt a little over the top to me, but it's fine. =)
steveklabnik
commented
Jan 2, 2017
I'm not totally opposed to this; I was thinking about making it future proof. That is, right now it's string in string out because it's a hack. It's really |
There was a problem hiding this comment.
I see we’re continuing the tradition of inconsistent capitalisation—nay, taking it to a new level, with mixed title and sentence case all in one line!
There was a problem hiding this comment.
Yeah "Derive" felt like something that deserved a capital here, maybe not, idk
Thanks for writing the doc. I can get the basic concept of proc-macro now, but still haunted by some questions.
|
There was a problem hiding this comment.
We should probably continue to document compiler plugins in the reference for the time being; they still exist & will for some time, and of course some project are heavily reliant on them.
There was a problem hiding this comment.
This isn't removing documentation; it's re-ordering it. I thought putting macros by example first was a better way of doing things.
There was a problem hiding this comment.
It looks like it went from Compiler plugins, Macros to Macros, Procedural Macros, shouldn't it be Macros, Procedural Macros, Compiler plugins?
There was a problem hiding this comment.
My understanding is that procedural macros are compiler plugins. That is, today "compiler plugins" are "using libsyntax to extend the compiler" but that's never going to be stable, only the new interface we're calling "procedural macros"
The names of all of this stuff has been very hard.
There was a problem hiding this comment.
That's correct, but we've implemented (and now stabilized) the new procedural macros interface only for controlling derive macros. Users who want to define custom attributes or function style procedural macros (without using a hack) are still using the never-to-be-stabilized compiler plugin interface.
Until we've fully implemented procedural macros, compiler plugins will still be used by these users and so we shouldn't drop whatever docs we have on it (even if we don't focus any attention on improving it).
There was a problem hiding this comment.
I didn't actually remove any of those docs generally, just the small description here. Let me fix it.
steveklabnik
commented
Jan 3, 2017
Sure, but it's much, much, much harder. The benefits are that syn and quote do parsing and generation for you.
This is the difference between macros 1.1 and macros 2.0, in a nutshell.
This is the entire API. That's it. There's nothing else to explain. It is extremely bare-bones. |
eb2c83f to
16f76c4Comparesteveklabnik
commented
Jan 3, 2017
Okay, I believe that I've addressed everyone's comments now, both on GitHub and in the text. I've squashed, since this is going to need to be backported, and want to make that easy. |
nikomatsakis
commented
Jan 3, 2017
Let's do it. @bors r+ |
bors
commented
Jan 3, 2017
📌 Commit eb2c83f has been approved by |
nikomatsakis
commented
Jan 3, 2017
@bors rollup |
16f76c4 to
c0efdbfComparesteveklabnik
commented
Jan 3, 2017
@bors: r=nikomatsakis rollup (#38770 (comment)) |
bors
commented
Jan 3, 2017
📌 Commit c0efdbf has been approved by |
…ikomatsakis Document custom derive. These are some bare-bones documentation for custom derive, needed to stabilize "macros 1.1", rust-lang#35900 The book chapter is based off of a blog post by @cbreeden, https://cbreeden.github.io/Macros11/ Normally, we have a policy of not mentioning external crates in documentation. However, given that syn/quote are basically neccesary for properly using macros 1.1, I feel that not including them here would make the documentation very bad. So the rules should be bent in this instance. So far, this PR includes only docs; @alexcrichton said in rust-lang#35900 that he'd be okay with landing them before stabilization; I don't mind either way.
alexcrichton
commented
Jan 4, 2017
@bors: rollup- p=1 Let's land this quickly so we can backport |
bors
commented
Jan 4, 2017
🔒 Merge conflict |
bors
commented
Jan 4, 2017
☔ The latest upstream changes (presumably #38783) made this pull request unmergeable. Please resolve the merge conflicts. |
These are some bare-bones documentation for custom derive, needed to stabilize "macros 1.1", rust-lang#35900 The book chapter is based off of a blog post by @cbreeden, https://cbreeden.github.io/Macros11/ Normally, we have a policy of not mentioning external crates in documentation. However, given that syn/quote are basically neccesary for properly using macros 1.1, I feel that not including them here would make the documentation very bad. So the rules should be bent in this instance.
c0efdbf to
3075c1fComparesteveklabnik
commented
Jan 4, 2017
@bors: r=alexcrichton p=1 |
bors
commented
Jan 4, 2017
📌 Commit 3075c1f has been approved by |
| moment, procedural macros need to be in their own crate. Eventually, this | ||
| restriction may be lifted, but for now, it's required. As such, there's a | ||
| convention; for a crate named `foo`, a custom derive procedural macro is called | ||
| `foo-derive`. Let's start a new crate called `hello-world-derive` inside our |
There was a problem hiding this comment.
I think the convention is foo_derive not foo-derive.
There was a problem hiding this comment.
package names are supposed to prefer - over _.
bors
commented
Jan 4, 2017
⌛ Testing commit 3075c1f with merge 8b9bc29... |
bors
commented
Jan 4, 2017
💔 Test failed - status-travis |
steveklabnik
commented
Jan 4, 2017
@bors: retry (travis is spurious) |
steveklabnik
commented
Jan 4, 2017
@bors: force |
bors
commented
Jan 5, 2017
⌛ Testing commit 3075c1f with merge 5d994d8... |
Document custom derive. These are some bare-bones documentation for custom derive, needed to stabilize "macros 1.1", #35900 The book chapter is based off of a blog post by @cbreeden, https://cbreeden.github.io/Macros11/ Normally, we have a policy of not mentioning external crates in documentation. However, given that syn/quote are basically neccesary for properly using macros 1.1, I feel that not including them here would make the documentation very bad. So the rules should be bent in this instance. So far, this PR includes only docs; @alexcrichton said in #35900 that he'd be okay with landing them before stabilization; I don't mind either way.
bors
commented
Jan 5, 2017
☀️ Test successful - status-appveyor, status-travis |
These are some bare-bones documentation for custom derive, needed
to stabilize "macros 1.1",
#35900
The book chapter is based off of a blog post by @cbreeden,
https://cbreeden.github.io/Macros11/
Normally, we have a policy of not mentioning external crates in
documentation. However, given that syn/quote are basically neccesary
for properly using macros 1.1, I feel that not including them here
would make the documentation very bad. So the rules should be bent
in this instance.
So far, this PR includes only docs; @alexcrichton said in #35900 that he'd be okay with landing them before stabilization; I don't mind either way.