Deprecation warning: You modified translatedString twice in a single render. This was unreliable in Ember 1.x and will be removed in Ember 2.0

[erangeles]

[erangeles]

2 failing tests, please do not review yet.

[notmessenger]

Reviewed 4 of 4 files at r1.
Review status: 1 of 4 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from the review on Reviewable.io

[erangeles]

@notmessenger fixed failing tests, ready to be reviewed.

[notmessenger]

Review status: 1 of 4 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

^{addon/components/sl-translate.js, line 59 [r5] (raw file):}

• Capitalize the first letter of the line
• Lower case the T and S in Translated String

---

^{addon/components/sl-translate.js, line 68 [r5] (raw file):}

• Is missing @function tag
• Is missing @returns tag
• Should not contain @type tag

---

^{addon/components/sl-translate.js, line 70 [r5] (raw file):}
Is in the wrong section of the document

---

^{tests/dummy/app/controllers/demo.js, line 8 [r5] (raw file):}
Add an empty line after this one

---

Comments from the review on Reviewable.io

[notmessenger]

"Update Values" button in demo app is not updating values in the template

---

Review status: 1 of 4 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[erangeles]

for tracking purposes.
handlebars-lang/handlebars.js#1050

[erangeles]

Review status: 1 of 5 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

^{addon/components/sl-translate.js, line 59 [r5] (raw file):}
Done.

---

^{addon/components/sl-translate.js, line 68 [r5] (raw file):}
Done.

---

^{addon/components/sl-translate.js, line 70 [r5] (raw file):}
Done.

---

^{tests/dummy/app/controllers/demo.js, line 8 [r5] (raw file):}
Done.

---

Comments from the review on Reviewable.io

[notmessenger]

Once you have made all of the modification to addon/components/sl-translate.js take another look at it and see if we still need as many moving pieces as well have to accomplish our goals. I feel like we might be able to simplify it further but it's hard to get a sense of given the number of comments I made that will result in code changes. Will need to see it afterwards to get a better picture.

---

Reviewed 1 of 3 files at r2, 1 of 4 files at r6, 1 of 1 files at r9.
Review status: 3 of 5 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

---

^{addon/components/sl-translate.js, line 38 [r8] (raw file):}
Is this property needed anymore after the refactor?

---

^{addon/components/sl-translate.js, line 43 [r8] (raw file):}
Is this description completely accurate anymore after the refactor?

---

^{addon/components/sl-translate.js, line 99 [r8] (raw file):}
Do not write this comment in the context of representing the differences between what was being done and what is now, but instead to explain what currently being done. As such it can be much shorter and succinct.

---

^{addon/components/sl-translate.js, line 106 [r8] (raw file):}
Does not follow https://github.com/softlayer/ember-style-guide#do-not-redefine-the-willinsertelement-didinsertelement-willclearrender-willdestroyelement-or-diddestroyelement-methods-or-do-so-with-full-knowledge-of-what-you-are-doing

---

^{addon/components/sl-translate.js, line 137 [r8] (raw file):}
Can this logic be replaced/combined with that from extractParameterKeys()and extractParameterKeys() be removed? extractParameterKeys() used to be needed because there were multiple methods (since removed) that were using the arrays it was populated.

---

^{addon/components/sl-translate.js, line 158 [r8] (raw file):}
Is this line needed? Doesn't the call to the same method in willRender() take care of things or am I not understanding the timing of things correctly?

---

^{addon/components/sl-translate.js, line 79 [r9] (raw file):}
What is the reason for these checks? Know you mentioned it briefly on the call but I haven't fully dug into it for understanding.

---

^{addon/components/sl-translate.js, line 84 [r9] (raw file):}
Shouldn't we keep this restricted to just numbers to enforce current behavior? It can then be relaxed once #6 is addressed.

---

^{tests/unit/components/sl-translate-test.js, line 42 [r9] (raw file):}
Knowing where we are heading with the tests - #208 - you don't have to bother adding this assertion if you don't want to.

---

^{tests/unit/components/sl-translate-test.js, line 48 [r9] (raw file):}
Comment can be removed. If the test needs more explanation or clarity then refine the description of the test.

---

^{tests/unit/components/sl-translate-test.js, line 63 [r9] (raw file):}
Are we manipulating properties like we were before? If not, change the comment. If not, is this test even any long necessary?

---

^{tests/unit/components/sl-translate-test.js, line 125 [r9] (raw file):}
The spacing employed in the array syntax passes linting?

---

Comments from the review on Reviewable.io

[notmessenger]

Reviewed 2 of 4 files at r6.
Review status: 4 of 5 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[notmessenger]

Reviewed 1 of 1 files at r7, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[erangeles]

Review status: 3 of 5 files reviewed at latest revision, 12 unresolved discussions, some commit checks pending.

---

^{addon/components/sl-translate.js, line 38 [r8] (raw file):}
no longer needed thus removed.

---

^{addon/components/sl-translate.js, line 43 [r8] (raw file):}
this prop is no longer needed thus removed

---

^{addon/components/sl-translate.js, line 99 [r8] (raw file):}
fixed doc block.

---

^{addon/components/sl-translate.js, line 106 [r8] (raw file):}
fixed.

---

^{addon/components/sl-translate.js, line 137 [r8] (raw file):}
refactored code.

---

^{addon/components/sl-translate.js, line 158 [r8] (raw file):}
removed.

---

^{addon/components/sl-translate.js, line 79 [r9] (raw file):}
this.attr is on hold, thus removed conditional.

---

^{addon/components/sl-translate.js, line 84 [r9] (raw file):}
fixed.

---

^{tests/unit/components/sl-translate-test.js, line 42 [r9] (raw file):}
forthcoming test push I will address #208 .

---

^{tests/unit/components/sl-translate-test.js, line 48 [r9] (raw file):}
removing in forthcoming test push.

---

^{tests/unit/components/sl-translate-test.js, line 63 [r9] (raw file):}
removing in forthcoming test push.

---

^{tests/unit/components/sl-translate-test.js, line 125 [r9] (raw file):}
yes it did.

---

Comments from the review on Reviewable.io

[erangeles]

forthcoming test push.

[notmessenger]

Reviewed 2 of 2 files at r14.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{addon/components/sl-translate.js, line 45 [r14] (raw file):}
This needs to be moved to the appropriate document section, per https://github.com/softlayer/ember-style-guide#file-structure-2

---

^{tests/unit/components/sl-translate-test.js, line 45 [r14] (raw file):}
extractParameterKeys() does not exist anymore

Think this entire test needs to be revisited, but will do so in #208

---

^{tests/unit/components/sl-translate-test.js, line 63 [r14] (raw file):}
This test case is skipping too many steps along the way. Based on the description, and start with the first part of it, I would expect to see that a call to setTranslatedString() sets the internalTranslatedString value and calls translateString(). And that's it. That tests setTranslatedString() as a single unit.

Then there would be a test case to test the translateString() logic in isolation. translatedString() would also have its own tests. How all of these pieces work together to form a single unit is the job of integration tests. Unit tests should test the output of code with known inputs and no external inputs/calls.

---

Comments from the review on Reviewable.io

[erangeles]

Review status: 3 of 7 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{addon/components/sl-translate.js, line 45 [r14] (raw file):}
Done.

---

^{tests/unit/components/sl-translate-test.js, line 45 [r14] (raw file):}
Done.

---

^{tests/unit/components/sl-translate-test.js, line 63 [r14] (raw file):}
added test for setTranslatedString, adding tests for translatedString()

---

Comments from the review on Reviewable.io

[notmessenger]

There needs to a test that asserts that this.setTranslatedString() is called when the willRender() event occurs.

---

Reviewed 3 of 4 files at r15, 2 of 2 files at r18.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

^{package.json, line 39 [r15] (raw file):}
Did this come about due to the installation of ember-sinon?

---

^{tests/unit/components/sl-translate-test.js, line 45 [r15] (raw file):}
Put the test description on its own line as well, so that the function definition doesn't get "lost"

---

^{tests/unit/components/sl-translate-test.js, line 137 [r18] (raw file):}
Wondered why $19 isn't also expected. Then realized that the regex was (originally too) setup to only accept single-digit values. It should be changed to accept any length of numerical values.

---

Comments from the review on Reviewable.io

[notmessenger]

^{package.json, line 57 [r18] (raw file):}
I recommend you install version 0.2.1, to align with the installed version in all the other add-on repos. The release notes for 0.3.0 say

Explicit QUnit support is removed in version 0.3.0. You should use ember-sinon-qunit instead.

which we need to research the impact of.

---

Comments from the review on Reviewable.io

[erangeles]

addition of "this.setTranslatedString() is called when the willRender() event occurs." test.

---

Review status: 4 of 7 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

^{package.json, line 39 [r15] (raw file):}
It was, removed since it was deemed needed.

---

^{package.json, line 57 [r18] (raw file):}
reverted version.

---

^{tests/unit/components/sl-translate-test.js, line 45 [r15] (raw file):}
done.

---

^{tests/unit/components/sl-translate-test.js, line 137 [r18] (raw file):}
Done. updated regex and test.

---

Comments from the review on Reviewable.io

[notmessenger]

Reviewed 3 of 3 files at r19, 1 of 1 files at r20.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

^{tests/unit/components/sl-translate-test.js, line 159 [r19] (raw file):}
Spelling of "evenet"

---

Comments from the review on Reviewable.io

[erangeles]

Review status: 6 of 7 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

^{tests/unit/components/sl-translate-test.js, line 159 [r19] (raw file):}
Done.

---

Comments from the review on Reviewable.io

[notmessenger]

Reviewed 1 of 1 files at r21.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[erangeles]

@notmessenger the "build" badge on our github repo is failing. I click on it and it says the build is passing in the 'build history' tab. not sure why the build badge isnt reflecting that.

[notmessenger]

@erangeles Because it looks at the default branch of the repo (which is master) and only updates itself when there is a push to the branch, which there hasn't been in some time. At some point between when there last was and the status was checked either there was a build failure due to either code or to the CI environment having an issue (most likely the case). When the EmberCLI-1.13.6 branch gets merged into master this should become resolved.