-
-
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
Reusing a constant's value in a macro #2388
Comments
What about call side syntax for this? Something like |
@jhass It's a good idea. But it would have to be syntax that doesn't conflict with the current one: |
What about using
Or maybe
|
Granted, it would require new syntax that then isn't passable to a macro. |
I think what @bcardiff suggests might work. Right now that is passed as a |
I have some doubts about what will happen with nested macros, maybe there is an issue in that use case, but I think sit down to think about it. But the |
That's why delegating to the resolved type transparently will always work, even with nested macros. I think I'd like to try that solution and see what happens :-) |
Ummm... well, as soon as I started implementing this I realized it won't work. For example all AST nodes have a |
How about a new node, or flag, for pure literal-consts, instead of generic Path? When a Path is assigned a literal, it gets LiteralConst or something along those lines from the parser. Then macro can automatically use the value for those and generic Paths are passed as is? |
This conversation was sparked when I found out that you can get a But I also found this silly workaround through |
I just realized something: macros are usually not made for 2 kinds of arguments (Path / constant), so if you use a macro made for constants you always have to use these brackets when calling, which is annoying (as is making a wrapped macro as demonstrated in @asterite's PR).
I don't understand this. How can you "pass a Path instead of the required type" when such solution presumes that you can ONLY pass a path (as it always was)? |
@BlaXpirit Take for example the For the other cases where the macro expects a type node, we can add a |
@asterite Oh, of course, this all makes sense. I spaced out and forgot that you can pass literals.
|
With the above commit this is now possible: macro include_constants(t)
{% for c in t.resolve.constants %}
{{c}} = {{t}}::{{c}}
{% end %}
end
enum E
A, B, C
end
class T1
include_constants E
end
p T1::A # works
p T1::B # works
p T1::C # works |
Expand macro expressions arguments before macro calls. Fixes #2388
Say we have a type and we want to provide both JSON and YAML mapping. We can do this:
In this case the mapping is exactly the same, so we'd want to refactor it like this:
However, that won't work. The problem is that the
mapping
macros expect aHashLiteral
as an argument, but now they receive aPath
(aPath
is something likeFoo::Bar::Baz
or justFoo
).An alternative is to do this:
but I'd say that's just a workaround. I'd really like the original refactor to work... somehow.
(A separate discussion is whether the way to define mappings is ok or not, maybe the mapping should be defined as attributes of instance variables... but it's a separate discussion, the point here is to reuse code in macros)
A different example: a macro that gets some info from a type, for example its instance variables:
Same problem:
type
is aPath
, not aTypeNode
denoting a type (the one you get if you use@type
inside a macro, of if you mention a type inside a macro, like writingSomeType
inside it, like if you do{{SomeType.instance_vars}}
).One solution is, if a method is not found in a
Path
(and right nowPath
has no macro methods), then it's solved to either its constant value or type. So, the above examples would work transparently, although it might be a bit confusing.Another solution is to make this explicit with a
resolve(...)
macro method, but in the end every macro would have to invoke this "just in case" someone passes a Path instead of the required type.I'm more inclined to implement the first solution, but do you know other solutions to this?
The text was updated successfully, but these errors were encountered: