implements namedBin for the declarative parser
#1365
Conversation
| @@ -56,6 +56,24 @@ proc extractFeatures(featureNode: PNode, conf: ConfigRef, hasErrors: var bool): | |||
| extractRequires(stmt, conf, requires, hasErrors) | |||
| result.add requires | |||
|
|
|||
| proc extractTableLiteral(n: PNode, conf: ConfigRef): Table[string, string] = | |||
| ## Extracts a table literal of the form {"key": "value"} or {"key": "value"}.toTable() | |||
There was a problem hiding this comment.
No, that's terrible. Instead only allow:
namedBin = {
"features_deps": "features_deps"
}There was a problem hiding this comment.
Thats allowed too, but not allowing toTable() would break when parsing the file with the vm
There was a problem hiding this comment.
How so? is namedBin an existing feature? Never heard of it...
There was a problem hiding this comment.
Yes, it is. i.e. nimbus uses it https://github.com/status-im/nimbus-eth2/blob/fad7b9a7133faf63efb552b6caaa3fb1554aba19/beacon_chain.nimble#L56
There was a problem hiding this comment.
Does the existing Nimble require the toTable? If not, don't support it and make the Nimbus people update their nimble file.
There was a problem hiding this comment.
there's no circular dep though .
There is nimble specific code in the nim compiler no?
There was a problem hiding this comment.
There is nimble specific code in the nim compiler no?
there's an agreement between the two to look in some nimble folders, but I think the best thing that can happen is that this code is removed in favor of nimble declaring to nim (via nimble setup) what it should do. ie whatever is there, is fairly contained and minimal
There was a problem hiding this comment.
It took a month long painstaking process to rip Nimble off the Nim compiler dependency for it used to import the Nim compiler as a library for Nimble evaluation. We won't go back to that.
There was a problem hiding this comment.
Didnt know that but as you know the compiler is used in the new parser anyways. Right now we have the bad parts of both worlds: the evaluation happens with an arbitrary nim different than the selected which we cant control. Maybe when the declarative parser is the new default we could workaround it by first selecting nim
There was a problem hiding this comment.
Using the parser is a whole different story than using Nim's VM. The parser's API is stable and small, the VM's is messy and it keeps us from replacing it with a better one.
No description provided.