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

Port {{concat}} helper test to ember-glimmer #12910

Merged
merged 1 commit into from
Feb 4, 2016

Conversation

chancancode
Copy link
Member

This is a 1:1 port of https://github.com/emberjs/ember.js/blob/master/packages/ember-htmlbars/tests/helpers/concat-test.js to the ember-glimmer test directory, with some minor cleanup. We don't have {{#if}} working yet (working on that next), so I split the "can be used as a sub-expression" test into two (which imo, seemed like a good refactor anyway).

The test is sym-linked back into ember-htmlbars as usual, so it is ran in both packages.

Given that this is 1:1 with the existing test, and it is ran in both packages, is it okay if we delete the existing version here? (I would like to do that, so we can easily keep track of what we have made ported.)

cc @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Feb 4, 2016

This seems to duplicate a few things:

  • packages/ember-glimmer/lib/helpers/-concat.js looks like a copy of packages/ember-htmlbars/lib/helpers/-concat.js
  • packages/ember-glimmer/lib/helper.js likes like a copy of packages/ember-htmlbars/lib/helper.js

Can these be symlinks instead?

@rwjblue
Copy link
Member

rwjblue commented Feb 4, 2016

Given that this is 1:1 with the existing test, and it is ran in both packages, is it okay if we delete the existing version here?

Yes, I think deleting is totally fine as we port the tests. The key thing to me is that the same tests have to be run in both contexts, and the new tests have to faithfully represent the intent of the existing tests (which I think you have done well here).

@chancancode
Copy link
Member Author

I symlinked the -concat.js (and renamed it to concat.js). I kept helper.js as is because they have some small differences (and will likely diverge more).

wycats added a commit that referenced this pull request Feb 4, 2016
Port `{{concat}}` helper test to `ember-glimmer`
@wycats wycats merged commit c0c0d83 into emberjs:master Feb 4, 2016
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