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

style: add prettier, lint-staged, husky and eslint rules #1120

Merged
merged 29 commits into from
May 2, 2023

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Apr 9, 2023

style only changes of PR #1119

It doesn't change any code, just re-formatting, only code outside examples/ and src/ are manually changed

you can verify it by these steps:

gh pr checkout 1120
# make sure you have up-to-date master branch
git checkout master -- src examples
npm i
npm run lint-fix
npm run format
git add .
git diff # should not showing any diff

eslintrc is copied from #1114 and prettier/husky/lint-staged is from #1109 (comment)

add eslint plugin eslint-plugin-simple-import-sort

(quotes of previous source code are mixed, so prettier config singleQuote true of false doesn't make much difference...)

@aldy505
Copy link
Contributor

aldy505 commented Apr 10, 2023

I've seen the code changes. #1119 should be merged first in order for this one to continue.

@trim21
Copy link
Contributor Author

trim21 commented Apr 10, 2023

I've seen the code changes. #1119 should be merged first in order for this one to continue.

The patch of this PR is inclued in #1119, if #1119 is merged then this PR could be close.

As I said in #1119

if you want code style config files to be split into another PR

personally I hope to merge this PR first then I rebase #1119 and then review/merge it

@prakashsvmx
Copy link
Member

Thank you @trim21 . I ran tests locally and looks good. 👍

suggestion : we could exclude CI config files from formatting

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

Just add ci files to prettierignore and split lint/build workflow to a seprated workflow file

prakashsvmx
prakashsvmx previously approved these changes Apr 12, 2023
Copy link
Member

@prakashsvmx prakashsvmx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

add eslint rule 'no-template-curly-in-string' for PR #1125

prakashsvmx
prakashsvmx previously approved these changes Apr 13, 2023
@trim21
Copy link
Contributor Author

trim21 commented Apr 13, 2023

@kannappanr @harshavardhana could you please review this pr?

We are trying to migrate source to TypeScript and want to add a formatter first for easier code review, this pr add formatter only and no code are changed, just reformatted

prakashsvmx
prakashsvmx previously approved these changes Apr 17, 2023
@trim21
Copy link
Contributor Author

trim21 commented Apr 19, 2023

resolve conflict

@trim21
Copy link
Contributor Author

trim21 commented Apr 20, 2023

@kannappanr @harshavardhana can you take a look?

@trim21
Copy link
Contributor Author

trim21 commented Apr 21, 2023

🤔any news on this?

@trim21
Copy link
Contributor Author

trim21 commented Apr 26, 2023

merge current master branch and resolve conflict

@trim21
Copy link
Contributor Author

trim21 commented Apr 26, 2023

rollup plugins are added unintendedly

@trim21
Copy link
Contributor Author

trim21 commented Apr 27, 2023

@harshavardhana there is no more feature/fix PR, would you mind review this PR now?

@trim21
Copy link
Contributor Author

trim21 commented Apr 30, 2023

merge current master branch and resolve conflict

@trim21
Copy link
Contributor Author

trim21 commented Apr 30, 2023

@prakashsvmx for fast review, this it what changed since your last approval

https://github.com/minio/minio-js/pull/1120/files/e1c7e3ffb2f7304e45229efafceeba32feac633f..8c3b7fb1b7d584f751bb2e3a7ed930d1ee523fac

I hope we can merge this soon so I don't need to keep merging branch again and again... meanwhile GitHub dismisses the approval, take it forever to get all reviewers' approval

Copy link
Member

@prakashsvmx prakashsvmx left a comment

Choose a reason for hiding this comment

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

Ran Tests Locally. All tests passing. 👍

@harshavardhana harshavardhana merged commit 5b233c9 into minio:master May 2, 2023
16 checks passed
@trim21 trim21 deleted the style branch May 7, 2023 11:44
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.

None yet

4 participants