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

[#1980] Standardise Array Style for Frontend Files #2084

Merged
merged 12 commits into from
Jan 31, 2024

Conversation

sopa301
Copy link
Contributor

@sopa301 sopa301 commented Jan 20, 2024

Fixes #1980

Proposed commit message

Standardise Array Usage

Array<T> and T[] can be used interchangeably in TypeScript. We can use
the same style consistently across the codebase to make it easier for
developers to read and understand array types.

In Vue, using T[] in defineComponent requires casting in the form of
"Array as PropType<T[]>", while Array<T> can be used as is. Therefore,
it would be more consistent to use Array<T> for .vue files.
Additionally, such a rule can be enforced more easily with eslint.

Let's include a rule for eslint to check array usage and change the
code to follow this rule.

Other information

@sopa301 sopa301 changed the title Standardise Array Style [#1980] Standardise Array Style Jan 20, 2024
@sopa301 sopa301 marked this pull request as ready for review January 20, 2024 04:45
@jonasongg jonasongg requested a review from a team January 20, 2024 05:10
Copy link
Contributor

@jonasongg jonasongg left a comment

Choose a reason for hiding this comment

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

Just a small nit, but might be better to avoid cluttering the commit with changes to other lines in .eslintrc.json!

@sopa301
Copy link
Contributor Author

sopa301 commented Jan 20, 2024

I've removed the unrelated lines. It was added by mistake by the IDE :P

@sopa301 sopa301 requested a review from jonasongg January 20, 2024 05:37
Copy link
Contributor

@jonasongg jonasongg left a comment

Choose a reason for hiding this comment

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

LGTM!

@sopa301 sopa301 changed the title [#1980] Standardise Array Style [#1980] Standardise Array Style for TS Files Jan 20, 2024
@ckcherry23
Copy link
Member

@sopa301

Interestingly, while TypeScript makes no distinction between the two styles, Vue (particularly in defineComponent) only accepts Array<T> directly, while requiring something like [] as T[] for the other style. I would like some input from someone with more experience in Vue on this.

For now, I have implemented the rule only for the TS files as the Vue side doesn't seem so clear-cut.

Thanks for your investigation regarding this! For defining arrays with the T[] syntax within Vue's defineComponent, you can try using Array as PropType<T[]>.

However, it does seem that Array<T> would be shorter and better for such use cases. If possible, switch to the Array<T> syntax for all array uses within defineComponent. Otherwise, we can standardise to Array as PropType<T[]>. Thanks!

@sopa301 sopa301 changed the title [#1980] Standardise Array Style for TS Files [#1980] Standardise Array Style for Frontend Files Jan 22, 2024
@sopa301 sopa301 requested a review from a team January 22, 2024 18:26
@sopa301
Copy link
Contributor Author

sopa301 commented Jan 22, 2024

@ckcherry23 Thank you for the review! I'm thinking using Array for all Vue files is more consistent and enforceable, but I can change only the defineComponent code if needed. Or we could go with Array as PropType<T[]>. I'll try to check if the latter is enforceable via eslint.

Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

@sopa301 This looks good to me! Thanks for your work!

@ckcherry23 ckcherry23 requested a review from a team January 22, 2024 18:33
@ckcherry23
Copy link
Member

@sopa301 Can you please resolve the merge conflicts?

@ckcherry23 ckcherry23 requested a review from vvidday January 28, 2024 04:10
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

LGTM, nice job

@ckcherry23 ckcherry23 merged commit 19f9e68 into reposense:master Jan 31, 2024
10 checks passed
Copy link
Contributor

The following links are for previewing this pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent use of T[] and Array<T> for arrays
4 participants