Skip to content

ZEPPELIN-235: jscs in zeppelin-web#229

Closed
prabhjyotsingh wants to merge 2 commits intoapache:masterfrom
prabhjyotsingh:ZEPPELIN-235
Closed

ZEPPELIN-235: jscs in zeppelin-web#229
prabhjyotsingh wants to merge 2 commits intoapache:masterfrom
prabhjyotsingh:ZEPPELIN-235

Conversation

@prabhjyotsingh
Copy link
Copy Markdown
Contributor

@prabhjyotsingh prabhjyotsingh commented Aug 19, 2015

ZEPPELIN-235: jscs in zeppelin-web
used following for .jscsrc

{
"preset": "google",
"maximumLineLength": {
"value": 120,
"allExcept": [ "comments", "regex"]
}
}

@corneadoug
Copy link
Copy Markdown
Contributor

corneadoug commented Aug 20, 2015

I guess there might be plenty of errors, do you need help cleaning them?

@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

prabhjyotsingh commented Aug 20, 2015

@corneadoug Intellij Idea fixed most if not all of the issues. However if build fails, or if there are errors in merging this to master, this will surly need help.

@corneadoug
Copy link
Copy Markdown
Contributor

corneadoug commented Aug 23, 2015

We might have to work on the configuration for indentation.

Example:

current formatting in this branch

angular.module('zeppelinWebApp').controller('ParagraphCtrl',
  function($scope, $rootScope, $route, $window, $element, $routeParams, $location, $timeout, $compile,
           websocketMsgSrv) {

Gives a 4 spaces prefix indent for the function code

One line formatting

angular.module('zeppelinWebApp').controller('ParagraphCtrl', function($scope, $rootScope, $route, $window, $element, $routeParams, $location, $timeout, $compile, websocketMsgSrv) {

Gives a 2 spaces prefix indent for the function code but obviously more than 120 characters line.

Super line formatting

angular
  .module('zeppelinWebApp')
  .controller('ParagraphCtrl',
              function($scope, $rootScope, $route, $window, $element, $routeParams, $location, $timeout, $compile,
                        websocketMsgSrv) {

or

angular
  .module('zeppelinWebApp')
  .controller('ParagraphCtrl',
              function($scope,
                        $rootScope,
                        $route,
                        $window,
                        $element,
                        $routeParams,
                        $location,
                        $timeout,
                        $compile,
                        websocketMsgSrv) {

Then I have jscs errors asking for a 16 spaces prefix indent for the function code.

In the end, I'm not sure which indenting is the best.

@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

prabhjyotsingh commented Aug 24, 2015

Sure giving a 4 spaces prefix indent for the function code, makes code look slightly more cleaner.

I thought of 120 characters in line, because in my view this should fit in most of the monitors. If not 120 characters what can be the right value?

But what I would prefer is using one of the standard template from the following list
https://github.com/jscs-dev/node-jscs/tree/master/presets

@corneadoug
Copy link
Copy Markdown
Contributor

corneadoug commented Aug 24, 2015

Our base is the google guideline, so using its prefix sounds good.
But I'm having some trouble when re-indenting the code in my text editor.
Especially when it comes to breaking down some lines.
So I think I'm gonna have to check how to do this more in the google rules, and see if it needs a few modifications. Maybe also checkout the other presets at the same time

@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

prabhjyotsingh commented Aug 24, 2015

Well I'm using Intellij Idea, and on pressing Control + Shift + A spawns up this menu, which does the job. I'm sure similar option should be available in other IDEs as well.

screen shot 2015-08-24 at 9 47 24 pm

If the plugin is not already installed here is how to:
https://www.jetbrains.com/idea/help/using-javascript-code-quality-tools.html#installJSCS

@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

prabhjyotsingh commented Aug 25, 2015

Can we start something basic, and then keep improvising it on the need basis?
Also, I thought "preset": "google" will take care of a lot of things.

@prabhjyotsingh
Copy link
Copy Markdown
Contributor Author

prabhjyotsingh commented Oct 21, 2015

Closing this, will open up again with fixed changes

@prabhjyotsingh prabhjyotsingh mentioned this pull request Oct 21, 2015
Closed
@prabhjyotsingh prabhjyotsingh mentioned this pull request Jul 6, 2016
Closed
asfgit pushed a commit that referenced this pull request Jul 8, 2016
@prabhjyotsingh
[ZEPPELIN-235] jscs in zeppelin-web
5556f81 
### What is this PR for?
Reopening the PR #229 for jscs in zeppelin-web, using following  in .jscsrc

```
{
  "preset": "google",
  "maximumLineLength": {
  "value": 120,
  "allExcept": [ "comments", "regex"]
  }
}
```

### What type of PR is it?
[Improvement]

### What is the Jira issue?
* [ZEPPELIN-235](https://issues.apache.org/jira/browse/ZEPPELIN-235)

### How should this be tested?
Try changing the indentation in any javascript file, and see an error being shown in the terminal on compile/build

### Questions:
* Does the licenses files need update? n/a
* Is there breaking changes for older versions? n/a
* Does this needs documentation? n/a

Author: Prabhjyot Singh <[email protected]>

Closes #1139 from prabhjyotsingh/ZEPPELIN-235 and squashes the following commits:

9cc3cde [Prabhjyot Singh] resove merge errors
a274efa [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-235
c16d2c7 [Prabhjyot Singh] addressing @corneadoug feedback
bffd134 [Prabhjyot Singh] revert 4 space changes for notebook.controller.js
02de55d [Prabhjyot Singh] jscs in zeppelin-web
PhilippGrulich pushed a commit to SWC-SENSE/zeppelin that referenced this pull request Aug 8, 2016
@prabhjyotsingh @PhilippGrulich
[ZEPPELIN-235] jscs in zeppelin-web
e938832 
### What is this PR for?
Reopening the PR apache#229 for jscs in zeppelin-web, using following  in .jscsrc

```
{
  "preset": "google",
  "maximumLineLength": {
  "value": 120,
  "allExcept": [ "comments", "regex"]
  }
}
```

### What type of PR is it?
[Improvement]

### What is the Jira issue?
* [ZEPPELIN-235](https://issues.apache.org/jira/browse/ZEPPELIN-235)

### How should this be tested?
Try changing the indentation in any javascript file, and see an error being shown in the terminal on compile/build

### Questions:
* Does the licenses files need update? n/a
* Is there breaking changes for older versions? n/a
* Does this needs documentation? n/a

Author: Prabhjyot Singh <[email protected]>

Closes apache#1139 from prabhjyotsingh/ZEPPELIN-235 and squashes the following commits:

9cc3cde [Prabhjyot Singh] resove merge errors
a274efa [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-235
c16d2c7 [Prabhjyot Singh] addressing @corneadoug feedback
bffd134 [Prabhjyot Singh] revert 4 space changes for notebook.controller.js
02de55d [Prabhjyot Singh] jscs in zeppelin-web
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@prabhjyotsingh @corneadoug