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 'route-path-style' rule #369

Merged
merged 1 commit into from Mar 18, 2019
Merged

Add new 'route-path-style' rule #369

merged 1 commit into from Mar 18, 2019

Conversation

bmish
Copy link
Member

@bmish bmish commented Jan 6, 2019

New rule:

  • Title: route-path-style
  • Description: Enforces usage of kebab-case (instead of snake_case or camelCase) in route paths.

Kebab-case looks like this URL: https://guides.emberjs.com/release/getting-started/core-concepts/

I'm assuming that kebab-case is the only route path format that we want to promote/enforce in Ember. This format seems to be a best practice / preference across the web, as even Google recommends it:

Consider using punctuation in your URLs. The URL http://www.example.com/green-dress.html is much more useful to us than http://www.example.com/greendress.html. We recommend that you use hyphens (-) instead of underscores (_) in your URLs.

@Turbo87 Turbo87 requested a review from rwjblue January 10, 2019 10:25
@Turbo87 Turbo87 changed the title feat: add new rule 'routes-path-style'. Add new 'routes-path-style' rule Jan 10, 2019
@bmish bmish changed the title Add new 'routes-path-style' rule Add new 'require-kebab-case-in-route-paths' rule Jan 11, 2019
@Turbo87
Copy link
Member

Turbo87 commented Mar 16, 2019

@bmish could you extract the changes to the existing rules to separate PRs? those changes make the review of the new rule quite a bit more complicated

@bmish
Copy link
Member Author

bmish commented Mar 16, 2019

@Turbo87 good point, I removed the cleanup of other rules, which should dramatically simplify the PR diff.

@Turbo87
Copy link
Member

Turbo87 commented Mar 18, 2019

@bmish I'm wondering if we should make this rule a little more generic. this is definitely a styling rule where different developers may have different opinions and it would be bad to have a separate rule for every style.

I was thinking that maybe we should rename this to route-path-casing (or even route-path-naming) with an optional style option that defaults to kebab case. We might even be able to pass a custom validator function to it to support any custom naming schemes 🤔

I think @rwjblue was a bit skeptical about suggesting that everything should be kebab case but if we make it configurable we might persuade him that this can be a useful rule 😉

@bmish
Copy link
Member Author

bmish commented Mar 18, 2019

@Turbo87 Agreed about making the rule more generic if we are fine with allowing different styles. So I renamed the rule to route-path-style. I'm not sure it's worth adding a configuration yet but we could absolutely do that later if we wanted to add more supported styles.

@bmish bmish changed the title Add new 'require-kebab-case-in-route-paths' rule Add new 'route-path-style' rule Mar 18, 2019
category: 'Best Practices',
recommended: false
},
fixable: null,
Copy link
Member

Choose a reason for hiding this comment

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

this should be reasonably easy to fix, right? but I guess this is what you meant with "changing the behavior"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Converting a string to kebab case should be trivial, but yeah, I'd be worried that an autofixer for this rule would go against the following eslint principle, as it could break existing links/URLs:

Avoid any fixes that could change the runtime behavior of code and cause it to stop working.

@Turbo87
Copy link
Member

Turbo87 commented Mar 18, 2019

@rwjblue I think this rule, in general, can be useful for some people so I'll go ahead and merge this. I don't think we should turn it on as a recommendation, but having it available for those people that would like to enforce this style (e.g. Square) seems reasonable to me.

@Turbo87 Turbo87 merged commit ce20118 into ember-cli:master Mar 18, 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

2 participants