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

chore(deps): update dependency eslint-plugin-prettier to v5 #4864

Merged

Conversation

edalex-yinzi
Copy link
Contributor

@edalex-yinzi edalex-yinzi commented Aug 29, 2023

Checklist
Description of change

To fix #4845.

Update prettier to v3.
Format all ts files.

@edalex-ian
Copy link
Member

Need to run eslint format:

image

@edalex-yinzi
Copy link
Contributor Author

Too many conflicts after merging gitlab branch. I probably will do another force push.

@edalex-yinzi edalex-yinzi force-pushed the fix/eslint-plugin-prettier-5.x branch 2 times, most recently from 50d56cc to 43f1b62 Compare September 1, 2023 06:36
@edalex-yinzi
Copy link
Contributor Author

Flaky test multipleAttachmentSections.

@edalex-ian
Copy link
Member

hmmmm, what's happened here @edalex-yinzi ? Before it failed on a handful of files, but suddenly you've gone and change 289 files!!

And I'm not sure are about the superfluous commas in the function definitions..... 🤔

@edalex-yinzi
Copy link
Contributor Author

hmmmm, what's happened here @edalex-yinzi ? Before it failed on a handful of files, but suddenly you've gone and change 289 files!!

And I'm not sure are about the superfluous commas in the function definitions..... 🤔

prettier is updated from v2 to v3, it might contain new rules. All these files are updated by npm run format.

@edalex-yinzi
Copy link
Contributor Author

I will do another force push and try format again.

@edalex-yinzi
Copy link
Contributor Author

Breaking changes on v3:

image

@edalex-yinzi edalex-yinzi force-pushed the fix/eslint-plugin-prettier-5.x branch from 43f1b62 to 3026a46 Compare September 7, 2023 00:50
@edalex-yinzi edalex-yinzi marked this pull request as draft September 7, 2023 05:12
@edalex-yinzi edalex-yinzi force-pushed the fix/eslint-plugin-prettier-5.x branch from 3026a46 to a7220e8 Compare September 7, 2023 05:21
@edalex-yinzi edalex-yinzi marked this pull request as ready for review September 10, 2023 22:58
@edalex-ian
Copy link
Member

Heya @edalex-yinzi ,

So basically this is ready for review, we just need to decide if we're happy with the extra commas. Is that correct?

Please address conflicts.

@edalex-yinzi edalex-yinzi force-pushed the fix/eslint-plugin-prettier-5.x branch from b227fd8 to 4ec3ace Compare October 2, 2023 05:09
@edalex-yinzi
Copy link
Contributor Author

Heya @edalex-yinzi ,

So basically this is ready for review, we just need to decide if we're happy with the extra commas. Is that correct?

Please address conflicts.

Yes.

@PenghaiZhang
Copy link
Contributor

I am not quite used to so many trailing commas, but I guess this has become the standard.

@PenghaiZhang
Copy link
Contributor

Hi @edalex-yinzi , can you resolve the conflict and merge develop to your branch and try again ?

@edalex-yinzi
Copy link
Contributor Author

Hi @edalex-yinzi , can you resolve the conflict and merge develop to your branch and try again ?

Sure.

@PenghaiZhang
Copy link
Contributor

Finally the build is OK. Do you want to review again @edalex-ian ?

@edalex-ian
Copy link
Member

The only thing I have @PenghaiZhang , is do we want to keep all those commas? Or should we tweak the rules like we've done for others?

The specific change is here (as well as how to disable it): https://prettier.io/blog/2023/07/05/3.0.0.html#change-the-default-value-for-trailingcomma-to-all-11479httpsgithubcomprettierprettierpull11479-by-fiskerhttpsgithubcomfisker-13143httpsgithubcomprettierprettierpull13143-by-sosukesuzukihttpsgithubcomsosukesuzuki

Why have trailing commas:

So with that it feels like it's the idiomatic style JS peeps want and hence prettier are aligning with that. So perhaps we should just go with it.

What do you think? If you're happy, feel free to merge. If you'd like to discuss, let me know.

@PenghaiZhang
Copy link
Contributor

The only thing I have @PenghaiZhang , is do we want to keep all those commas? Or should we tweak the rules like we've done for others?

The specific change is here (as well as how to disable it): https://prettier.io/blog/2023/07/05/3.0.0.html#change-the-default-value-for-trailingcomma-to-all-11479httpsgithubcomprettierprettierpull11479-by-fiskerhttpsgithubcomfisker-13143httpsgithubcomprettierprettierpull13143-by-sosukesuzukihttpsgithubcomsosukesuzuki

Why have trailing commas:

So with that it feels like it's the idiomatic style JS peeps want and hence prettier are aligning with that. So perhaps we should just go with it.

What do you think? If you're happy, feel free to merge. If you'd like to discuss, let me know.

Although I do not like these trailing commas, maybe it's just because I am not used to it. And also because this has become the idiomatic style, I think it's better to accept it now so that we would not have too much trouble in the future when these commas become more important.

@PenghaiZhang PenghaiZhang merged commit 780fa45 into openequella:develop Nov 1, 2023
7 checks passed
@edalex-yinzi edalex-yinzi deleted the fix/eslint-plugin-prettier-5.x branch March 6, 2024 23:50
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.

3 participants