Skip to content
This repository has been archived by the owner on Apr 25, 2018. It is now read-only.

Add the rest of the nodes #81

Merged
merged 5 commits into from
Jul 29, 2017
Merged

Add the rest of the nodes #81

merged 5 commits into from
Jul 29, 2017

Conversation

davepagurek
Copy link
Member

This makes two new base classes:

  • Boolean, for things in the condition of a ScopeFn
  • ScopeFn, which takes a NodePtr in addition to a Json in its to_html method. It transforms the context and passes it to the NodePtr.

Some refactors done in the process:

  • Refactored TagStatement and Nested into one class
  • Took the const out of the Json typedef so that we can use it mutably to build up a new context in a ScopeFn. After we're good with all the nodes, we should change the to_html signature to take in a const Json (we were going to refactor it anyway to add const to the method.) Added this to Clean up grammar #78

Things left to do after this:

  • I can't make the whole thing because there are some other build errors right now, so probably when we fix the existing ones, we'll discover new build errors in this code.
  • Our current code for repeated rules will need to be changed, unfortunately. Fix repeated nodes #80

@davepagurek davepagurek requested a review from armcburney July 28, 2017 04:13

return NodePtr(new TagStatement(tag_name, id_name, class_names, body, attributes));
return NodePtr(
new TagStatement(tag_name, id_name, class_names, body, attributes, nested));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious - why did you put this on a new line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was getting a little long

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added another parameter to the list, btw, which is what made me think to bump it down a line

@@ -37,7 +35,7 @@ il_lit_str_content <- !'"' .

escaped <- '\\' .

tag_statement <- tag $identifier< id_name? > $classes< class_name* > ~space* $body< text_content? > $attributes< attributes? >
tag_statement <- tag id_name? class_name* ~space* text_content? attr_list? (~nl ~lbrace ~nl ROOT ~rbrace ~nl)?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 💯

bool truthy(Json &context) const override;

private:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove this blank newline please?


BooleanPtr lhs;
BooleanPtr rhs;
Operator op;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And add a blank newline after line 23?

key_name(key_name)
{}

std::string Each::to_html(NodePtr body, Json &context) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is kinda big, and hard to read - do you think we could break this up into a few smaller private methods belonging to the Each class?


std::string Scope::to_html(Json &context) {
return scope_fn.to_html(body);
return scope_fn->to_html(body, context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍

private:
std::string collection_name;
std::string val_name;
std::string key_name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a blank newline after this statement?

@@ -6,13 +6,15 @@ TagStatement::TagStatement(
std::string id,
std::vector<std::string> classes,
NodePtr body,
NodePtr attributes
NodePtr attributes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we renamed the attributes rule to the attr_list rule here. Should we call this attr_list in this class as well as here to be consistent?


private:
bool negated;
BooleanPtr expr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a newline after here?


unary_expr <- $negated< ('not' space+)? > ($val< variable_name > / '(' space* $val< boolean_expr > space* ')')
unary_expr <- ('not' space+)? (variable_name / ~'(' ~space* boolean_expr ~space* ~')')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


boolean_expr <- binary_expr / unary_expr

binary_expr <- $lhs< unary_expr > space+ $op< 'and' / 'or' > space+ $rhs< boolean_expr >
binary_expr <- unary_expr ~space+ ('and' / 'or') ~space+ boolean_expr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


each <- 'each' space* $collection< variable_name > space* 'as' space* $val_name< variable_name > $indexed< (',' space* $key_name< variable_name >)? >
each <- ~'each' ~space* variable_name ~space* ~'as' ~space* ~variable_name (~',' ~space* ~variable_name)? >
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@armcburney armcburney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work 🙌 - just a few minor things, which I commented above.

Copy link
Member

@armcburney armcburney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll start using an autoformatter for C++ once we get the grammar working.

Copy link
Member

@armcburney armcburney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 :shipit:

@davepagurek davepagurek merged commit 69cbe86 into develop Jul 29, 2017
@davepagurek davepagurek deleted the scopes branch July 29, 2017 21:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants