Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Incorrect RegExp in export-name rule leads to superfluously permissive #632

Closed
IllusionMH opened this issue Nov 4, 2018 · 5 comments · Fixed by #642
Closed

Incorrect RegExp in export-name rule leads to superfluously permissive #632

IllusionMH opened this issue Nov 4, 2018 · 5 comments · Fixed by #642
Labels
Difficulty: Medium People with non-trivial experience in TSLint should be able to send a pull request for this issue. Status: Accepting PRs Type: Breaking Change Type: Bug
Milestone

Comments

@IllusionMH
Copy link
Contributor

IllusionMH commented Nov 4, 2018

As was discovered in #623 (comment) RegExp in export-name rule has issue with improperly escaped . as well as missing check for line start/end. These problems leads to several problems.

For class name SomeClass it allows:

  1. Wrong . escape: SomeClassButAnyTextHere.tsx, SomeClassWithoutExtension
  2. No end of line check: SomeClass.ts.or.not
  3. No start of line check: LooksLikeSomeClass.ts
  4. Everything combined NotSureThatSomeClassIsSomewhereInThis.long.filename

According to description I would expect only SomeClass.ts or at least SomeClass.suffix.tsx (but better to make it as option).

Existing test with a problem ExportNameRulePassingTestInput2.tsx ends on 2.tsx but class name doesn't have 2 et the end.

What do you think about cases above and which should be disallowed by this rule?

@JoshuaKGoldberg JoshuaKGoldberg added Type: Bug Status: Accepting PRs Difficulty: Medium People with non-trivial experience in TSLint should be able to send a pull request for this issue. Type: Breaking Change labels Nov 5, 2018
@JoshuaKGoldberg
Copy link

Agreed, this is an existing bug that should be fixed as suggested; the author likely meant to escape the . properly.

@IllusionMH
Copy link
Contributor Author

IllusionMH commented Nov 5, 2018

What about cases 2 and 3?

@JoshuaKGoldberg
Copy link

JoshuaKGoldberg commented Nov 5, 2018

  1. No end of line check: SomeClass.ts.or.not

When would this case be typically hit? SomeClass.spec.tsSomeClass.stub.ts, perhaps? My understanding of the RegExp's intent is that it was meant to allow (name of the exported thing) plus (any extension). It makes sense that we would consider .spec.ts to be the extension. Although the technical file extension is anything after the last ., this rule's logic would be anything after the first ..

  1. No start of line check: LooksLikeSomeClass.ts

Yes, this looks like a bug.

Edit: yes, .stub.ts would be more realistic

@IllusionMH
Copy link
Contributor Author

Not sure that .spec.ts files should export class, but may be useful for something like .stub.ts.
If required, strict check for "extension part" might be added as option after feedback.

Will fix 1 and 3.

@IllusionMH
Copy link
Contributor Author

Actually with fixed check for start of the line there are 80 (why not 93?) errors because of export class Rule ... in this repo 🤣
Does in make sense to add this fix to 6.0? Might be breaking for someone.
Implementation and tests almost finished.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Difficulty: Medium People with non-trivial experience in TSLint should be able to send a pull request for this issue. Status: Accepting PRs Type: Breaking Change Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants