Conversation
stasm
left a comment
stasm
left a comment
There was a problem hiding this comment.
Nice work, @unclenachoduh! Thank you for working on this and for refactoring all the tests!
I have a few minor comments (see below) as well as 3 general ones:
- We'll need some tests for the
compoundAPI. I can help you get started with a basic test file, if you'd like. - I think with these changes, we can remove the
getMessagemethod now. - (Minor) Let's take this opportunity to write out comments as full sentences, with a capital letter at the front and a fullstop at the end :)
I think this is very close to being good to land. Let me know if you're available to address these comments. I'll be happy to help with any of them, too! Thanks again!
| return null; | ||
| } | ||
|
|
||
| let attr = message.attrs[parts[1]]; |
There was a problem hiding this comment.
To keep it consistent with the parts.length === 1 branch above, perhaps you could add an if (typeof attr === "string") fast path? I'm not sure they're needed and I'll do some perf testing when I get a chance. For now it would be best to keep both branches as similar to each other as possible.
There was a problem hiding this comment.
I'm not sure where to place this fast path and which return it correlates to. Should this go in line 229?
|
@unclenachoduh Do you know if you'll be available in the coming weeks to finish this PR? I'd like to land it in early February. |
|
Hey @stasm I can get on this weekend to resolve the changes you've asked for. I'm a little busy right now, so I don't think I'll be able to write the new tests for |
|
I'll be happy to help with the tests. I'd like to land this next week; let me know if you're still available to apply my feedback. I understand that you're busy so no worries if not. |
|
I think I still have one of these changes pending. Take a look at the changes. |
| const msg = bundle.getMessage('foo'); | ||
| assert.equal(typeof msg, 'string'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
It would probably be better to mark this test as skipped with test.skip(...) rather than just removing it.
| const msg = bundle.getMessage('bar'); | ||
| assert(Array.isArray(msg)); | ||
| }); | ||
|
|
There was a problem hiding this comment.
It would probably be better to mark this test as skipped with test.skip(...) rather than just removing it.
|
I rather like this API, but I think the More specifically, I think |
|
I'd like to finish @unclenachoduh's work and land this change soon. To clean things up a bit, I'll squash all commits from this PR and I'll open a new PR keeping attribution of the code written so far. |
With
I'll file new issues about these, as you suggested. Thanks! |
|
I have no other use cases in mind for |
|
I opened a new PR to continue working on this: #360. I updated and rebased the code and fixed a few issues. I'm going to close this PR now. |
3 participants
Fix format for issue #208