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

@typescript-eslint/no-namespace #703

Merged
merged 5 commits into from
Aug 9, 2023

Conversation

metreniuk
Copy link
Contributor

@metreniuk metreniuk commented Aug 9, 2023

Overview
Fixes the no-namespace rule for #503

Help needed

  • I'm not sure how to better adjust the Tester to support running tests with different extensions, so I did the bare minimum and would appreciate guidance how to make it actually good. My current approach is just running a separate test/snapshot for tests that require a different extension. I was thinking adding it inside the options json, but it seemed unintuitive to use.
  • I removed the guard around typescript declarations inside crates/oxc_semantic/src/builder.rs, but I'm not sure why it was in the first place.

Notes

  • Added a typescript-eslint rule generator. Didn't support filename since had trouble adjusting the Tester module.

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic labels Aug 9, 2023
@metreniuk metreniuk changed the title @typescript eslint/no namespace @typescript-eslint/no-namespace Aug 9, 2023
@Boshen
Copy link
Member

Boshen commented Aug 9, 2023

We don't need to lint .d.ts because they are either machine generated or manually crafted.

@Boshen
Copy link
Member

Boshen commented Aug 9, 2023

Since the namespace syntax works on ordinary TypeScript files, perhaps we don't need to change anything around filenames?

@metreniuk
Copy link
Contributor Author

metreniuk commented Aug 9, 2023

Since the namespace syntax works on ordinary TypeScript files, perhaps we don't need to change anything around filenames?

In the original rule there is an option allowDefinitionFiles that allows namespaces in d.ts, if we don't lint those I can just remove the option. Wdyt?

@Boshen
Copy link
Member

Boshen commented Aug 9, 2023

Let's keep the parsed value and , we just don't use it for now.

Since the namespace syntax works on ordinary TypeScript files, perhaps we don't need to change anything around filenames?

In the original rule there is an option allowDefinitionFiles that allows namespaces in d.ts, if we don't lint those I can just remove the option. Wdyt?

Sounds good to me! Maybe keep the allowDefinitionFiles value in the code but comment out the relevant test cases?

@github-actions github-actions bot removed the A-semantic Area - Semantic label Aug 9, 2023
@metreniuk metreniuk requested a review from Boshen August 9, 2023 13:52
@Boshen Boshen force-pushed the @typescript-eslint/no-namespace branch from a7f6f93 to 7f33a4f Compare August 9, 2023 14:56
@Boshen Boshen merged commit f8358a1 into oxc-project:main Aug 9, 2023
@Boshen
Copy link
Member

Boshen commented Aug 9, 2023

Thank you! Love the addition to rule gen!

@metreniuk metreniuk deleted the @typescript-eslint/no-namespace branch August 9, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants