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

prefer-module: allow module as TSIndexSignature names or TSTypeAliasDeclaration ids #2209

Merged
merged 5 commits into from
Oct 29, 2023
Merged

prefer-module: allow module as TSIndexSignature names or TSTypeAliasDeclaration ids #2209

merged 5 commits into from
Oct 29, 2023

Conversation

ST-DDT
Copy link
Contributor

@ST-DDT ST-DDT commented Oct 27, 2023

Fixes #2208

I tried adding a test for that, but was unable to do that (maybe because it requires ts support):

  message: `␊
    > 1 | type ModuleRegistry = { [module: string]: string };␊
        |      ^ Parsing error: Unexpected token ModuleRegistry`,                                                                                                            
  }                                                                                                                                                                          

  » |      ^ Parsing error: Unexpected token ModuleRegistry
  » verify (file:///test/utils/snapshot-rule-tester.mjs:138:9)
  » file:///test/utils/snapshot-rule-tester.mjs:169:23

So I used https://github.com/faker-js/faker/pull/2510/files#diff-7170563e0d86e93e8997b4acb856e5e494809a0ef41b9f4386f26e14bc961908R19 to test that.

@ST-DDT ST-DDT changed the title fix(prefer-module): fix TSIndexSignature module names fix(prefer-module): allow module as TSIndexSignature names Oct 27, 2023
@fisker
Copy link
Collaborator

fisker commented Oct 27, 2023

Take this as example

Put your code in valid part.

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 28, 2023

Take this as example

Thanks for the help.
Is one test sufficient or do we need more?

@fisker
Copy link
Collaborator

fisker commented Oct 28, 2023

Added more, but feel free to add more.

Thanks!

@sindresorhus
Copy link
Owner

Tests are failing

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 28, 2023

fisker added an additional case where the type name is module.
The additional (failing) test, is also an (expectedly) valid and similar case, but it is different from the issue this PR originally tried to fix.

I'm not sure how I can fix that. Is there a way to run only a specific test? Maybe then I can try to debug it.

@fisker
Copy link
Collaborator

fisker commented Oct 29, 2023

I'll take a look.

@fisker fisker changed the title fix(prefer-module): allow module as TSIndexSignature names prefer-module: allow module as TSIndexSignature names or TSTypeAliasDeclaration ids Oct 29, 2023
@sindresorhus sindresorhus merged commit ea94b3b into sindresorhus:main Oct 29, 2023
21 checks passed
@ST-DDT ST-DDT deleted the fix/prefer-module/ts-index-signature branch October 29, 2023 16:50
@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 30, 2023

@fisker It looks like we missed a case:

type Data = {
  [module in 'foo']: number;
};

e.g.

type Data = {
  [module in keyof string]: number;
};

I could prepare a PR for that, but for that I need to know how I can run a specific test(case) only.

@fisker
Copy link
Collaborator

fisker commented Oct 30, 2023

I need to know how I can run a specific test(case) only.

Currently we don't support it yet. But it should be possible if we upgrade eslint-ava-rule-tester https://github.com/jfmengels/eslint-ava-rule-tester/releases/tag/v4.1.0

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 30, 2023

I used a workaround, aka deleting all tests except for the one I wanted and created a PR with the fix:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prefer-module should not report module name in types
3 participants