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

fix(type-utils): file variant of TypeOrValueSpecifier uses canonical filenames instead of lowercasing #6781

Merged
merged 4 commits into from
Jun 16, 2023
Merged

fix(type-utils): file variant of TypeOrValueSpecifier uses canonical filenames instead of lowercasing #6781

merged 4 commits into from
Jun 16, 2023

Conversation

marekdedic
Copy link
Contributor

PR Checklist

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @marekdedic!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@netlify
Copy link

netlify bot commented Mar 27, 2023

Deploy Preview for typescript-eslint failed.

Name Link
🔨 Latest commit 788c620
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/648c81b77271d2000826ca42

@marekdedic
Copy link
Contributor Author

marekdedic commented Mar 27, 2023

Hi,
since this was previously an issue on their machine, I am tagging @JoshuaKGoldberg in particular (I cannot assign you as a reviewer) to verify this works for them, thanks!

@marekdedic marekdedic marked this pull request as ready for review March 27, 2023 22:13
@nx-cloud
Copy link

nx-cloud bot commented Mar 27, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 788c620. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #6781 (e5a7c9a) into v6 (da95d62) will decrease coverage by 0.25%.
The diff coverage is 100.00%.

❗ Current head e5a7c9a differs from pull request most recent head 788c620. Consider uploading reports for the commit 788c620 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##               v6    #6781      +/-   ##
==========================================
- Coverage   87.75%   87.50%   -0.25%     
==========================================
  Files         372      376       +4     
  Lines       12813    12938     +125     
  Branches     3800     3821      +21     
==========================================
+ Hits        11244    11322      +78     
- Misses       1193     1231      +38     
- Partials      376      385       +9     
Flag Coverage Δ
unittest 87.50% <100.00%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/type-utils/src/TypeOrValueSpecifier.ts 89.65% <100.00%> (-0.67%) ⬇️

... and 36 files with indirect coverage changes

@bradzacher bradzacher added the bug Something isn't working label Apr 10, 2023
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Yeah I think this is very reasonable 👍. Just, it needs some test coverage please?

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Apr 16, 2023
@marekdedic
Copy link
Contributor Author

Well, I added a test for the normalization, however, I can't really test for capitalization - the test would then fail on some platforms, right? Does it pass on your machine?

@JoshuaKGoldberg JoshuaKGoldberg removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 17, 2023
@@ -1,3 +1,4 @@
import { getCanonicalFileName } from '@typescript-eslint/typescript-estree';
Copy link
Member

Choose a reason for hiding this comment

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

@bradzacher just confirming on architecture things - is there a better dependency setup we can use here?

Copy link
Member

Choose a reason for hiding this comment

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

No this is probably the best way to do it and ensures we keep things in sync with the parser's logic.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Yeah I see what you mean - testing this is hard. But what's here so far looks good to me, thanks!

Since this exports a previously internal helper I'll defer to its author for a second review.

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval One team member has approved this PR; a second should be enough to merge it label Apr 17, 2023
@JoshuaKGoldberg JoshuaKGoldberg merged commit 5095d05 into typescript-eslint:v6 Jun 16, 2023
37 of 43 checks passed
@marekdedic marekdedic deleted the fixed-TypeOrValueSpecifier-casing branch June 17, 2023 17:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval One team member has approved this PR; a second should be enough to merge it bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants