ParametersParser refacor #119
Conversation
|
Can you please unsubscribe me from these emails. From: Pavan Kumar Sunkara <notifications@github.commailto:notifications@github.com> WIP You can merge this Pull Request by running git pull https://github.com/apiaryio/snowcrash pksunkara/refactor Or view, comment on, or merge it at: Commit Summary
File Changes
Patch Links:
— Confidentiality Notice: This email message, including attachments, is for the sole use of the intended recipient(s) and may contain confidential and privileged information. Any unauthorized review, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. |
|
@psunkara Please click on "Unwatch" on the top right of the repo. Thanks |
| @@ -110,7 +113,7 @@ namespace snowcrash { | |||
| Value exampleValue; | |||
|
|
|||
| /** Enumeration of possible values */ | |||
| Collection<Value>::type values; | |||
There was a problem hiding this comment.
@pksunkara we have @alikh31 working over this interface – it would be really good if we avoid changes in this file unless absolutely necessary (e.g. the case with symbol table). Again we will revisit this file later but no need to do this at the moment. Please use typedef Collection<Value>::type Values; only "internally" on in the Values parser for now.
| // REQUIRE(blueprint.resourceGroups[0].resources[0].parameters[0].defaultValue == "42"); | ||
| //} | ||
|
|
||
| // TODO: How to warn for this? |
There was a problem hiding this comment.
@zdne Forgot to ask you today. I am wondering where to do this warning. In ParameterParser, while running nestedSectionType ?
There was a problem hiding this comment.
The fact that Values: is not recognized should be reported and ignored by the base implementation of SectionProcessor – processUnexpectedNode()
Looking at SectionProcessorBase:: processUnexpectedNode – sorry if I have forgot something, but didn't we agree on having it default implementation as warn & skip? (report ignoring and move to next node)
There was a problem hiding this comment.
Shouldn't this be considered as description node? Why are we processing it as unexpected node?
There was a problem hiding this comment.
Basically, I am asking whether we need to check for Values\n when running the nestedSectionType in ParameterParser?
There was a problem hiding this comment.
Shouldn't this be considered as description node? Why are we processing it as unexpected node?
This is good point! The only reason for this would be to warn user about the mistake. It is quite annoying when you think you wrote a keyword and it ends up (without warning) in the description.
However this should be handled more conceptually. So while leaving this to on the parser will for not worsen the situation for writer, I agree.
We should address this in #65
| SectionParserHelper<Parameter, ParameterParser>::parse(source, ParameterSectionType, report, parameter); | ||
|
|
||
| REQUIRE(report.error.code == Error::OK); | ||
| REQUIRE(report.warnings.empty()); |
There was a problem hiding this comment.
@zdne Due to the parser model, the Values: node is being identified as descriptionNode therefore not calling processUnexpectedNode. What do you want me to do here?
Other than that, this PR is ready to be merged once you review the latest commit.
|
@pksunkara Last 2 commits look OK. Is this still WIP or it is good to merge? |
WIP