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

2731: Add linting rule for arrays #2970

Merged
merged 6 commits into from
Nov 6, 2024
Merged

2731: Add linting rule for arrays #2970

merged 6 commits into from
Nov 6, 2024

Conversation

simomps
Copy link
Contributor

@simomps simomps commented Oct 24, 2024

Short description

Add a linting style for arrays

Proposed changes

  • All arrays displayed as arrayName[]
  • other array style changed according to new rule

Side effects

  • None

Testing

Check that all settings still work.

Resolved issues

Fixes: #2731


Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Congrats to your first PR, nicely done 🚀 Just one small comment regarding the yarn.lock, I think this is otherwise good to go!

tools/yarn.lock Outdated Show resolved Hide resolved
@simomps simomps changed the title 2731 linting rule arrays 2731: Add linting rule for arrays Oct 24, 2024
Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Thank you 👍

Copy link

codeclimate bot commented Oct 24, 2024

Code Climate has analyzed commit cf8c5ad and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 94.7% (50% is the threshold).

This pull request will bring the total coverage in the repository to 74.2%.

View more on Code Climate.

Copy link
Contributor

@LeandraH LeandraH left a comment

Choose a reason for hiding this comment

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

Very cool, thank you very much for cleaning up 🧽

I have found two more places where we still use the Array type, one is Selector.ts in the e2e-tests, the other one in helpers.spec.ts in native, might be worth a check why the linter doesn't find that. But then again, maybe not 😂

Copy link
Contributor

@LeandraH LeandraH left a comment

Choose a reason for hiding this comment

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

I'll approve it already since I'll be on vacation for a bit and don't want to block anything :)

@steffenkleinle
Copy link
Member

Very cool, thank you very much for cleaning up 🧽

I have found two more places where we still use the Array type, one is Selector.ts in the e2e-tests, the other one in helpers.spec.ts in native, might be worth a check why the linter doesn't find that. But then again, maybe not 😂

Actually I am quite sure that this is something different. The two occurrences mentioned by you are both referring to
something like the following:

const keys = new Array<string>()

These are initialization of a new array instance (of type string), opposed to something like the following which actually throws an error in both cases:

const keys: Array<string> = new Array<string>()

I therefore think we are actually good to go here :)

@LeandraH
Copy link
Contributor

LeandraH commented Nov 4, 2024

Woooops, you're right, sorry!

@simomps simomps merged commit 4efedf6 into main Nov 6, 2024
9 checks passed
@simomps simomps deleted the 2731-linting-rule-arrays branch November 6, 2024 14:30
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.

Add linting rule to use only one style of typing arrays
3 participants