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

Add new no-unnecessary-service-injection-argument rule #378

Merged
merged 1 commit into from Mar 16, 2019
Merged

Add new no-unnecessary-service-injection-argument rule #378

merged 1 commit into from Mar 16, 2019

Conversation

bmish
Copy link
Member

@bmish bmish commented Jan 17, 2019

@bmish bmish requested a review from Turbo87 January 17, 2019 04:02
@Turbo87 Turbo87 changed the title feat: add new rule 'no-unnecessary-service-injection-argument'. Add new no-unnecessary-service-injection-argument rule Jan 17, 2019
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

nice work! 👍

package.json Outdated Show resolved Hide resolved
@rwjblue
Copy link
Member

rwjblue commented Jan 17, 2019

I disagree with some of the examples 😺

  myServiceName: service('my-service-name')

^ should absolutely be allowed, it avoids needless runtime camelization <-> dasherization from happening in the resolver and is a good thing.

Making the rule focus on single word injections removes this concern...

@bmish
Copy link
Member Author

bmish commented Jan 18, 2019

@rwjblue thanks for the feedback. I have updated the rule to only disallow exact name matches, meaning dasherized names will now be allowed as an argument.

Out of curiosity, is the performance hit from camelization <-> dasherization actually meaningful? I assumed it would negligible, and that we should prefer to write the cleanest code instead.

@bmish bmish requested a review from rwjblue February 27, 2019 18:42
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks! (and sorry for the delay 😞)

@Turbo87 Turbo87 merged commit e4df30c into ember-cli:master Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants