Skip to content
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

Support else if #58

Merged
merged 1 commit into from
Jun 19, 2018
Merged

Support else if #58

merged 1 commit into from
Jun 19, 2018

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Jun 14, 2018

To learn more about Go I thought it would be cool to implement else if support.

What do you think about this approach?

Codeclimate is failing, but I can fix that later if I know I'm on the right track.

parser/parser.go Outdated

ifElseExp.Block = p.parseBlockStatement()

expression.ElseIf = append(expression.ElseIf, ifElseExp)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block is the same as the original IF, and can be extracted to a new func, to keep it DRY.

@stanislas-m
Copy link
Member

Just a random thought: since plush syntax is mostly inspired by erb, how about using the same keyword? (so elsif instead of else if)

That way, we still can use erb syntax highlighting for plush.

@markbates
Copy link
Member

Honestly I never liked that elsif syntax. I would rather else if, personally.

Copy link
Member

@markbates markbates left a comment

Choose a reason for hiding this comment

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

Overall it’s good. Clean it up a bit, and it’s a good solid contribution. Thanks.

@ruudk
Copy link
Contributor Author

ruudk commented Jun 14, 2018

I agree, I don't like the elsif because I like to more explicit, and write things out. So either elseif or else if

I will clean up the PR, thanks for your feedback 🙌

@ruudk
Copy link
Contributor Author

ruudk commented Jun 14, 2018

And also the reason for choosing else if was that it's the same in Go :)

@stanislas-m
Copy link
Member

That's a valid point. :)

@ruudk ruudk force-pushed the add-else-if branch 3 times, most recently from 9ff5d84 to ed30d28 Compare June 14, 2018 16:29
@ruudk
Copy link
Contributor Author

ruudk commented Jun 14, 2018

I cleaned up the PR. Woud love to get your feedback again :)

@ruudk
Copy link
Contributor Author

ruudk commented Jun 19, 2018

Rebased the PR with latest master. Is there anything that needs to be done before we can merge this? Please let me know, Happy to make more changes :)

@markbates
Copy link
Member

Love this! Thank you!

@markbates markbates merged commit 2ec029f into gobuffalo:master Jun 19, 2018
@stanislas-m stanislas-m mentioned this pull request Jul 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants