-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 class_* accessor macros #3658
Conversation
Personally I would much rather alter the accessor macros to allow |
@RX14 I'd prefer that too but since |
Looks like |
@RX14 I thought about that, but what does it mean by itself? |
@asterite just a TypeDeclaration with an invalid left side. We already use some syntax that lexes but will never compile (unused variable etc.) for macros. I imagine that a |
@RX14 I still like the conciseness of |
Is there really any reason why the old syntax exists? It's much less flexible in that you can't add type restrictions or defaults. |
@RX14 what do you mean by "old syntax"? |
|
:foo was the first syntax it was supported, then, when some type restrictions were wanted to be supported the idea of using type declarations I wouldn't mind dropping I understand the motivation of allowing |
6143ca1
to
1baa5f4
Compare
@Sija I have to review this. Even though I kind of agree with the name, reusing all the logic and having macros inside macros obfuscates the code quite a bit. But I guess it's harmless. I'll review it soon :-) |
@asterite Sure, even GH doesn't want to display the diff from default ;) I agree it ain't look gr8, but that was the only sensible way I've found to implement this feature without blatant copy-pasting all of the code once moar... |
@asterite If this is GTG I'd send next PR simplifying macros in question if that's OK with you. It would save some bytes and simplify the code. |
@Sija Don't worry, I'll soon review the code and probably merge it. These macros are almost fundamental and they won't change over time, so it's not that problematic if we use nested macros here. |
{% | ||
macro_prefix = prefixes[0] | ||
method_prefix = prefixes[1] | ||
var_prefix = prefixes[2] |
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.
@Sija We can do this:
macro_prefix = prefixes[0].id
method_prefix = prefixes[1].id
var_prefix = prefixes[2].id
Then everywhere below we don't need to use .id
and code is shorter and clearer (and slightly faster). What do you think?
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.
gr8! I've pushed changes with your suggestion applied :)
@Sija I reviewed this and I think it's really good. I'm glad we can use macros inside comments to generate nice docs :-) I just made a tiny comment for improvement. After that I'll merge this. |
1baa5f4
to
4d3eb92
Compare
@Sija Thank you! 💙 |
Along the lines of #444 (comment)