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: Enable ESLint for apps and fix all errors #46082

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jun 24, 2024

Summary

Nevertheless this causes a huge amount of new warnings. Previously the shell script for directories to lint was wrong it was generating all app names to lint, but was missing the apps/ prefix. Causing only core to be linted.

Checklist

@susnux susnux added 3. to review Waiting for reviews ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) labels Jun 24, 2024
@susnux susnux added this to the Nextcloud 30 milestone Jun 24, 2024
@susnux susnux requested a review from ShGKme June 24, 2024 22:11
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

LGTM! There are npm build changes though.

@nickvergessen
Copy link
Member

Nice find 🙈

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Thanks!

Checked all the changes, looks good.
I expected much more changes 👀

The only thing I'm working about is removing the import. Having a file it is not easily possible to say if removing this line is safe. And strictly, each file with dialogs must have dialog styles.

import '@nextcloud/password-confirmation/dist/style.css'

apps/federatedfilesharing/src/external.js Outdated Show resolved Hide resolved
Comment on lines +16 to +17
"lint": "eslint $(for appdir in $(ls apps); do if ! $(git check-ignore -q $appdir); then printf \"apps/$appdir \"; fi; done) core --no-error-on-unmatched-pattern",
"lint:fix": "eslint $(for appdir in $(ls apps); do if ! $(git check-ignore -q $appdir); then printf \"apps/$appdir \"; fi; done) core --no-error-on-unmatched-pattern --fix",
Copy link
Contributor

Choose a reason for hiding this comment

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

It's been worrying me for a year 🙈

@susnux susnux force-pushed the chore/enable-eslint branch 4 times, most recently from b7cdf76 to df92bea Compare July 1, 2024 11:51
@susnux
Copy link
Contributor Author

susnux commented Jul 1, 2024

/compile

@susnux susnux force-pushed the chore/enable-eslint branch from b816e37 to 91f4df6 Compare July 2, 2024 10:14
@susnux
Copy link
Contributor Author

susnux commented Jul 2, 2024

@skjnldsv skjnldsv removed the 3. to review Waiting for reviews label Jul 4, 2024
@skjnldsv skjnldsv added the 2. developing Work in progress label Jul 4, 2024
@susnux susnux force-pushed the chore/enable-eslint branch from 91f4df6 to e626183 Compare July 5, 2024 16:59
@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Jul 5, 2024
@susnux susnux force-pushed the chore/enable-eslint branch from e626183 to 29c6ab3 Compare July 7, 2024 12:24
@skjnldsv
Copy link
Member

skjnldsv commented Jul 9, 2024

Can we do that just after branch off 31 please 🙏

@susnux susnux force-pushed the chore/enable-eslint branch from 29c6ab3 to ccb97bd Compare July 9, 2024 11:24
susnux and others added 4 commits July 9, 2024 17:13
Nevertheless this causes a huge amount of new warnings.
Previously the shell script for directories to lint was wrong it was generating all app names to lint,
but was missing the `apps/` prefix. Causing only `core` to be linted.

Co-authored-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the chore/enable-eslint branch from ccb97bd to b0f6686 Compare July 9, 2024 15:27
@susnux susnux merged commit 6d8a7a1 into master Jul 9, 2024
167 checks passed
@susnux susnux deleted the chore/enable-eslint branch July 9, 2024 17:04
@blizzz blizzz mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants