feat(ApplicationLauncher): added event params to onSearch and onFavorite #321
Conversation
| const rule = require('../../../lib/rules/v5/applicationLauncher-updated-params'); | ||
| const propNames = ['onFavorite', 'onSearch']; | ||
|
|
||
| const ruleRunner = (prop) => { |
There was a problem hiding this comment.
could we just do propNames.forEach( (prop) => { directly? so I don't have to wonder what's going on until i scroll to the bottom 😆
There was a problem hiding this comment.
What's wrong with having some adventure every now and then? 😂
There was a problem hiding this comment.
That being said, given the temporary nature of codemods I wonder a bit about extracting these event param tests into a reusable helper function.
I'd typically be against that sort of thing since it decreases test readability, but that tradeoff would probably be worth doing for codemods since we won't be maintaining them long term and it would speed up development.
WDYT?
There was a problem hiding this comment.
if it's set up right and we trust the helpers, it shouldn't reduce readability.. might make sense to have another helper in the tests dir, up to you if you wanna take a shot at that
gitdallas
left a comment
gitdallas
left a comment
There was a problem hiding this comment.
re-request if you plan to create a test helper, but otherwise lgtm
|
First swing at a helper added, not my proudest work.... but it gets the job done. WDYT @gitdallas ? |
|
@wise-king-sullyman - it's very specific, down to the "_event" being hard coded in.. but does the job for reducing a lot of duplicate code in v4-v5 codemods i think. i don't think we need to export the get..Tests functions. should we then use this for where it can be so far? |
|
Yeah my idea was for it to be a counterpart specifically for the |
|
maybe we'll deal with opening it up if we run into another case down the road. but should we use this where it can be now? |
|
Reverted the helper in this PR and created #334 for the test helper. Once either of these are merged I'll update the other as needed. |
…ers" This reverts commit 337ebfe.
wise-king-sullyman
force-pushed
the
application-launcher-callbacks-updated
branch
from
186c091 to
a0cec06
Compare
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Could we add to the test.tsx file as well?
3 participants
Closes #313