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

Add eslint global nextcloud config #17263

Merged
merged 2 commits into from
Oct 1, 2019
Merged

Add eslint global nextcloud config #17263

merged 2 commits into from
Oct 1, 2019

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Sep 25, 2019

Crazy lots of cleanup! 😑
I found some prettu weird code out there. 🙈

  • Leftovers from copy/pasting
  • Undeclared variables
  • Unchanged vue templates names

Lots of cleanup left to do, but will be in a follow-up

  • Put settings components into folders
  • Cleanup apps management app that is very hard to understand
  • Cleanup the accessibility app too
  • Cleanup the updatenotification and simplify it as it's also hard to understand (split into small components)
  • Implement proper escape-html
  • Cleanup user management
  • Fix all the javascript copyright comments that do not start with /** but /*
  • ... anything else ?

@rullzer
Copy link
Member

rullzer commented Sep 25, 2019

so. It all looks sane. I'm just afraid to press approve on such a huge PR...

@go2sh
Copy link
Contributor

go2sh commented Sep 25, 2019

Just for curiosity: Is it really necessary to remove all semicolons? I know nearly no guide, which recommends automatic semicolon insertion (ASI). I looked up Google, AirBNB, W3 School, Mozilla. Only StandardJS does. Even the Nextcloud docs says: Complete every statement with a ; ;-)

@skjnldsv
Copy link
Member Author

@go2sh that's a very long debate :p
I think you can find as many posts about why semicolon looks better and are less error-prone than article about why removing them is also less error prone and looks better 🤷‍♀️

In the end, to quote someone: "Discussing about it is opinionated and will induce an endless debate which is not what we want." 😉
https://standardjs.com/#i-disagree-with-rule-x-can-you-change-it

@skjnldsv skjnldsv force-pushed the enhancement/eslint branch 2 times, most recently from 2126057 to 1154a6a Compare September 26, 2019 07:10
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Files like settings/js/vue-settings-apps-users-management.js have like 25k lines added. Did you compile in dev mode? The bundles should not change their size actually.

core/js/merged-share-backend.js Show resolved Hide resolved
@skjnldsv
Copy link
Member Author

skjnldsv commented Sep 26, 2019

Files like settings/js/vue-settings-apps-users-management.js have like 25k lines added. Did you compile in dev mode? The bundles should not change their size actually.

I don't think so! 🤔

EDIT: yes it is :)
Let's wait for #17239 and then I'll rebuild properly!

@go2sh
Copy link
Contributor

go2sh commented Sep 26, 2019

@skjnldsv i dont want to start a discussion. I just thought semicolon was the way to go in nextcloud

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 1, 2019

Bump! I'd like to quickly get this so I don't have to rebase crazy things and also we can catch stuff early for 18 :)

@nickvergessen
Copy link
Member

Well I guess we should just rebase and instant merge this
if there are further issues we fix them afterwards, but rebasing +14k -12k is not going to work out.

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 1, 2019

Well I guess we should just rebase and instant merge this
if there are further issues we fix them afterwards, but rebasing +14k -12k is not going to work out.

let me rebase one last time then

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 1, 2019

All rebased!
Please do not push anything! Review this asap 🙏

@nickvergessen nickvergessen merged commit 7dc5bba into master Oct 1, 2019
@nickvergessen nickvergessen deleted the enhancement/eslint branch October 1, 2019 15:20
@gary-kim
Copy link
Member

gary-kim commented Oct 1, 2019

While understandable for this one since it would be very wasteful to keep re-basing a change like this, I'd like to ask that merging without reviews not become a pattern. Especially for changes as large as this one.

@nickvergessen
Copy link
Member

Surely not becoming a pattern 👍

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.

None yet

6 participants