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

fix(rome_js_analyze): useShorthandArrayType should handle nested ReadonlyArray and not report to TsObjectType #4354

Merged
merged 3 commits into from
Apr 11, 2023
Merged

fix(rome_js_analyze): useShorthandArrayType should handle nested ReadonlyArray and not report to TsObjectType #4354

merged 3 commits into from
Apr 11, 2023

Conversation

unvalley
Copy link
Contributor

@unvalley unvalley commented Apr 6, 2023

Summary

Closes #4349 #4361

This pr adds modification for nested ReadonlyArray (#4349 ) and reduce false positive (#4361 ).
Currently, our useShorthandArrayType follows typescript-eslint/array-type (array-simple option).

And the rule should not report TsObjectType in a type Array or ReadonlyArray.

Example

// issue 4349
type Before = ReadonlyArray<ReadonlyArray<T>>
type After = readonly (readonly T[])[]

// issue 4361
type AcceptedObjectType = Array<{x: string, y: string}>

Test Plan

cargo test -p rome_js_analyze -- use_shorthand_array_type

Changelog

  • [ ] The PR requires a changelog line

Documentation

  • [ ] The PR requires documentation
  • [ ] I will create a new PR to update the documentation
    I updated lint rule doc a bit (describes typescript-eslint rule equivalent).

@netlify
Copy link

netlify bot commented Apr 6, 2023

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fff746c
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/643512788f694a0008c959f7
😎 Deploy Preview https://deploy-preview-4354--docs-rometools.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the A-Linter Area: linter label Apr 6, 2023
@unvalley unvalley changed the title fix(rome_js_analyze): useShorthandArrayType to handle nested Readonly… fix(rome_js_analyze): useShorthandArrayType to handle nested ReadonlyArray Apr 6, 2023
@unvalley unvalley marked this pull request as ready for review April 7, 2023 15:07
@jpike88
Copy link

jpike88 commented Apr 8, 2023

This rule is not behaving the same as the eslint equivalent. If I enable the Rome rule, my codebase lights up light xmas tree with loads of false positives.

Eslint rule below:

'@typescript-eslint/array-type': [
			'error',
			{
				default: 'array-simple',
			},
		],

See below screenshot from eslint typescript playground with the above rule enabled, correctly identifying the bottom example only. Rome would treat both examples as violating the rule.
Screenshot 2023-04-08 at 11 32 07 am

@unvalley
Copy link
Contributor Author

unvalley commented Apr 8, 2023

Thank you, @jpike88. I have fixed the issue at f20fa12. Our next step should be to create test cases for typescript-eslint/array-type/array-simple properly if current test cases are not enough. This will require another PR.

@jpike88
Copy link

jpike88 commented Apr 8, 2023

got it ill make seperate issue

@unvalley unvalley changed the title fix(rome_js_analyze): useShorthandArrayType to handle nested ReadonlyArray fix(rome_js_analyze): useShorthandArrayType should handle nested ReadonlyArray and not report to TsObjectType Apr 8, 2023
@unvalley unvalley changed the title fix(rome_js_analyze): useShorthandArrayType should handle nested ReadonlyArray and not report to TsObjectType fix(rome_js_analyze): useShorthandArrayType should handle nested ReadonlyArray and not report to TsObjectType Apr 8, 2023
@ematipico
Copy link
Contributor

@unvalley this PR requires a changelog line, could you please add it?

@unvalley
Copy link
Contributor Author

@ematipico I updated it.

@ematipico ematipico merged commit e97ddf6 into rome:main Apr 11, 2023
@unvalley unvalley deleted the fix/use-short-hand-array-action branch April 11, 2023 08:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Invalid useShorthandArrayType applied fix
3 participants