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

[New] sort-prop-types: give errors on TS types #3615

Merged

Conversation

akulsr0
Copy link
Contributor

@akulsr0 akulsr0 commented Aug 9, 2023

This PR is an attempt to solve #3578

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #3615 (70c21f1) into master (378de4c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 70c21f1 differs from pull request most recent head cab612f. Consider uploading reports for the commit cab612f to get more accurate results

@@           Coverage Diff           @@
##           master    #3615   +/-   ##
=======================================
  Coverage   97.63%   97.64%           
=======================================
  Files         132      132           
  Lines        9312     9337   +25     
  Branches     3400     3423   +23     
=======================================
+ Hits         9092     9117   +25     
  Misses        220      220           
Files Changed Coverage Δ
lib/rules/sort-prop-types.js 98.70% <100.00%> (+0.43%) ⬆️

... and 8 files with indirect coverage changes

@ljharb
Copy link
Member

ljharb commented Aug 11, 2023

Seems like there's some uncovered added lines; can you add tests that hit them?

@ljharb ljharb marked this pull request as draft August 11, 2023 18:21
@ljharb
Copy link
Member

ljharb commented Aug 12, 2023

Looks like there's still a few failing tests.

@akulsr0
Copy link
Contributor Author

akulsr0 commented Aug 12, 2023

Yep, tried to debug this. It seems for node 8/9 and eslint 6, the variableUtil.findVariableByName util is unable to grab the type node. Not sure why this is happening

@akulsr0 akulsr0 marked this pull request as ready for review August 12, 2023 08:43
@akulsr0
Copy link
Contributor Author

akulsr0 commented Aug 14, 2023

@ljharb I have fixed the previous failing tests.

@ljharb
Copy link
Member

ljharb commented Aug 15, 2023

ok, I rebased this, but I think this needs to go behind an option, otherwise it would be a breaking change.

@ljharb ljharb force-pushed the feat/sort-prop-types-with-ts-types branch from 2b8d443 to da1cc91 Compare August 15, 2023 04:57
@ljharb ljharb changed the title feat(enhancement): enable sort-prop-types to give errors on ts types [New] sort-prop-types: give errors on TS types Aug 15, 2023
@ljharb ljharb marked this pull request as draft August 15, 2023 04:57
lib/rules/sort-prop-types.js Outdated Show resolved Hide resolved
tests/lib/rules/sort-prop-types.js Outdated Show resolved Hide resolved
lib/rules/sort-prop-types.js Show resolved Hide resolved
lib/rules/sort-prop-types.js Outdated Show resolved Hide resolved
lib/rules/sort-prop-types.js Show resolved Hide resolved
tests/lib/rules/sort-prop-types.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the feat/sort-prop-types-with-ts-types branch from 329c495 to 121c89a Compare August 17, 2023 05:25
@ljharb ljharb marked this pull request as ready for review August 17, 2023 05:27
@ljharb ljharb force-pushed the feat/sort-prop-types-with-ts-types branch from 121c89a to cab612f Compare August 17, 2023 05:29
@ljharb ljharb merged commit cab612f into jsx-eslint:master Aug 17, 2023
286 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants