Skip to content

build: add tslint rule for consistent file name casing #14536

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

Merged
merged 1 commit into from
Dec 18, 2018

Conversation

crisbeto
Copy link
Member

Adds a tslint rule to enforce that the casing on our filenames is consistent. The tslint rule files are excluded, because their names have to be in camel case and end with Rule.

@crisbeto crisbeto added pr: merge safe target: patch This PR is targeted for the next patch release labels Dec 15, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 15, 2018
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Why not just use "file-name-casing": [true, "kebab-case"], and either:

  • Exclude the tools/tslint-rules directory
  • Or if we argue that we still want to run other rules against our own custom rules, we can just add a tslint disable comment for the file-naming casing. That's still a win and avoids duplicating of existing rules IMO.

@crisbeto
Copy link
Member Author

crisbeto commented Dec 15, 2018

I did it because:

  • The logic is pretty simple and I doubt we'd need to do much maintenance on it.
  • I wanted to keep linting all of the rule files.
  • We have a lot of rule files. Aside from the custom tslint rules, we also have the schematic rules. That means having to add those disable comments in a lot of places and having to remember to do it whenever another gets added.

@devversion
Copy link
Member

devversion commented Dec 15, 2018

I kind of see your point, but I'm hesitant because we can simply avoid having another rule.

  1. The logic is simple, but just adding // tslint:disable-rule:file-name-casing only to our custom rules seems simpler to me (47loc vs ~5loc)
  2. We can still do that with the approach above
  3. True, but if we forget about it, tslint will automatically show it as an failure and we'll know about it. Also most likely all rules are copy-pasted from an existing rule.

It just feels wrong to me to add a custom rule for something that can be easily replaced by an official rule. Especially since that's basically why tslint's disable comment feature exists.

@crisbeto
Copy link
Member Author

It not just 5 files though. These are the files that would fail the check:

attributeSelectorsStringLiteralRule.ts
attributeSelectorsStylesheetRule.ts
attributeSelectorsTemplateRule.ts
classInheritanceCheckRule.ts
classNamesIdentifierRule.ts
cssSelectorsStringLiteralRule.ts
cssSelectorsStylesheetRule.ts
cssSelectorsTemplateRule.ts
elementSelectorsStringLiteralRule.ts
elementSelectorsStylesheetRule.ts
elementSelectorsTemplateRule.ts
inputNamesStylesheetRule.ts
inputNamesTemplateRule.ts
checkTemplateMiscRule.ts
outputNamesTemplateRule.ts
propertyNamesAccessRule.ts
constructorSignatureCheckRule.ts
methodCallsCheckRule.ts
checkClassInheritanceMiscRule.ts
checkClassNamesMiscRule.ts
checkImportsMiscRule.ts
checkPropertyNamesMiscRule.ts
checkTemplateMiscRule.ts
rippleSpeedFactorAssignmentRule.ts
rippleSpeedFactorTemplateRule.ts
missingRollupGlobalsRule.ts
noExposedTodoRule.ts
noHostDecoratorInConcreteRule.ts
noImportSpacingRule.ts
noPrivateGettersRule.ts
noUnescapedHtmlTagRule.ts
requireBreakingChangeVersionRule.ts
requireLicenseBannerRule.ts
rxjsImportsRule.ts
settersAfterGettersRule.ts
validateDecoratorsRule.ts

It'll start looking even weirder when we have to start placing that tslint:disable comment on top of some of these files that have a license header. It's pretty easy to miss the error in your IDE, because it only highlights the first character of the file.

@devversion
Copy link
Member

devversion commented Dec 15, 2018

Ah, I didn't realize that this also affects the schematic rules because I thought we exclude the ng-update folder within tslint.json.

It's true that it's easier to maintain the rule instead of a list like that then.

tslint.json Outdated
@@ -129,6 +129,11 @@
"./tools/package-tools/rollup-globals.ts",
"src/+(lib|cdk|material-examples|material-experimental|cdk-experimental)/!(schematics)**/*.ts"
],
"file-name-casing-scoped": [
true,
// Exclude lint rule file since they have to always be camel cased and end with `Rule`.
Copy link
Member

Choose a reason for hiding this comment

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

We should say plural: tslint rule files?

@devversion
Copy link
Member

devversion commented Dec 15, 2018

Actually the official tslint rule landed support for custom filename casing some days ago.

See: palantir/tslint@a43cfdc

This could spare us the additional custom rule and we can still get all benefits from a custom rule. It's just not released yet. I'm fine getting this in the meanwhile (even though it's urgent) or we just wait.

Adds a tslint rule to enforce that the casing on our filenames is consistent. The tslint rule files are excluded, because their names have to be in camel case and end with `Rule`.
@crisbeto
Copy link
Member Author

I've left in a TODO to consider replacing the rule once those changes are released.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Dec 16, 2018
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn merged commit 9e05af3 into angular:master Dec 18, 2018
josephperrott pushed a commit to josephperrott/components that referenced this pull request Jan 14, 2019
Adds a tslint rule to enforce that the casing on our filenames is consistent. The tslint rule files are excluded, because their names have to be in camel case and end with `Rule`.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants