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} blocks in @first, @last #143

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support {:else} blocks in @first, @last #143

wants to merge 3 commits into from

Conversation

claviska
Copy link

@claviska claviska commented Jan 29, 2017

Simple PR to add support for {:else} in @first and @last helpers, per linkedin/dustjs#720.

  • Passes all tests locally: 575 tests, 837 assertions, 0 failures, 0 skipped
  • Docs still need to be updated (is there a place to submit PRs for documentation?)

@claviska
Copy link
Author

Not sure why the Travis build is failing, but the error seems unrelated. Any ideas?

Copy link
Contributor

@sethkinast sethkinast left a comment

Choose a reason for hiding this comment

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

For style purposes, could you follow the pattern in the filter function with skip and nonternary return values?

@sethkinast
Copy link
Contributor

The helpers build uses loose versioning for the Jasmine dep and it's changed out from under you, not your fault. Would you mind adding a test?

Once this is merged you can open a PR against the gh-pages branch of the main Dust repo for the docs.

Thanks a bunch for the PR!

@claviska
Copy link
Author

Thanks. The format is consistent now and I added two tests. They're my first unit tests so feel free to suggest changes. 🙃

Copy link
Contributor

@sethkinast sethkinast left a comment

Choose a reason for hiding this comment

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

Looks great!

@sethkinast
Copy link
Contributor

Nice, thanks for taking the time to send a PR! :)

@sethkinast
Copy link
Contributor

@jimmyhchan #138 will fix the test failures, I'll try to work on it soon...

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.

2 participants