search

owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.38k stars 2.06k forks source link

Indentation coding style reported by Scrutinizer #7407

Closed jancborchardt closed 10 years ago

jancborchardt commented 10 years ago

I get some indentation issues reported, for example in apps/files_external/js/settings.js, core/js/router.js and core/js/share.js

I can’t seem to see what’s wrong with the indentation there. When I put it through http://jshint.com, there’s nothing wrong with the indentation. http://jslint.com complains to use spaces instead of tabs. If I change it to spaces, there are no errors on the line.

So I presume Scrutinizer also reports these isses because of the tab indentation. Is there a means to say that we actually use tabs so these false positives are not reported? @DeepDiver1975

jancborchardt commented 10 years ago

This issue still exists. @PVince81 @DeepDiver1975 any idea if that’s an issue in Scrutinizer/JSHint and we can disable that check? Otherwise these non-errors pollute the log.

PVince81 commented 10 years ago

@jancborchardt I just checked locally with JSHint and these are indeed valid errors.

It's just that no one bothered to reformat these files properly yet (like settings.js in external storage that has wrong indentation).

jancborchardt commented 10 years ago

But they are formatted correctly as far as I can see. What’s the error? We aren’t gonna convert everything to spaces, are we?

PVince81 commented 10 years ago

Many rows in settings.js have a mix between tabs and spaces, 1 tab + 1 space. These should be converted to tabs only.

This is probably very old code that isn't modified a lot, that's why we didn't think of fixing them I guess.

PVince81 commented 10 years ago

I can fix these if you like, I need a break from PR reviews...

jancborchardt commented 10 years ago

I’ll fix those files cause they show up in my Scrutinizer issues. ;) Need to wash my name clean. You can look at your Scrutinizer issues: https://scrutinizer-ci.com/g/owncloud/core/issues/master?selectedAuthors%5B0%5D=pvince81%40owncloud.com&selectedAuthors%5B1%5D=PVince81%40yahoo.fr&orderField=path&order=asc ;)

jancborchardt commented 10 years ago

@PVince81 hm, maybe it’s better if you look into them after all. Just looked at apps/files_external/settings.js for example and there is only one single whitespace issue. No spaces or tabs mixed.

jancborchardt commented 10 years ago

At least partly fixed: https://github.com/owncloud/core/pull/7569

PVince81 commented 10 years ago

Do you have jshint running locally using our .jshintrc ? That one should give you the warnings.