Skip to content
This repository has been archived by the owner on Jul 12, 2020. It is now read-only.

Add eslint #29

Merged
merged 8 commits into from
May 24, 2018
Merged

Add eslint #29

merged 8 commits into from
May 24, 2018

Conversation

acjh
Copy link

@acjh acjh commented May 23, 2018

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Other, please explain: Code quality

What is the rationale for this request?

Consistency.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

How do I pick one? 😭

var s = ""

should be

const s = '';

Is there anything you'd like reviewers to focus on?

-

Testing instructions:

@acjh
Copy link
Author

acjh commented May 23, 2018

The intention is to eventually negate (!) all files under # MarkBind/vue-strap in .eslintignore.

@yamgent
Copy link
Member

yamgent commented May 24, 2018

The lint does not work when I copied over searchbar.vue and made some random changes. No errors were shown.

PS C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\vue-strap> npm run lint

> vue-strap@1.1.36 lint C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\vue-strap
> eslint . --ext .vue; exit 0

PS C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\vue-strap>

EDIT:
I managed to get it working only if exit 0 was removed from the lint script in package.json. I.e.:

"lint": "./node_modules/.bin/eslint . --ext .vue"

],
'max-len': ['error', { 'code': 110 }],
'operator-linebreak': ['error', 'before'],
'quote-props': 'off',
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider adding 'semi': 'off', since it looks like we are not enforcing the semi-colon terminator style?

Copy link
Author

Choose a reason for hiding this comment

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

Let's enforce it.

package.json Outdated
@@ -78,6 +82,7 @@
"builddocs": "webpack --progress --hide-modules && set NODE_ENV=production webpack --progress --hide-modules",
"docs": "webpack-dev-server --inline --hot --quiet --host 0.0.0.0",
"gpages": "gh-pages -d .",
"lint": "./node_modules/.bin/eslint . --ext .vue; exit 0",
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in an earlier comment, need to remove exit 0, otherwise it will always pass :P.

@yamgent
Copy link
Member

yamgent commented May 24, 2018

The linter seems to be working fine, I managed to fix most styling issues in searchbar.vue, but I am not sure whether we should consider disabling these three rules:

  10:31  error  Expected to return a value in arrow function
                             array-callback-return
  16:13  error  The body of a for-in should be wrapped in an if statement to filter unwanted properties from the prototype
                             guard-for-in
  16:13  error  for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array
                             no-restricted-syntax

✖ 3 problems (3 errors, 0 warnings)

A sample code that generates these errors:

<script>
const array = [1, 2, 3]
const dict = { 'a': 1, 'b': 2, 'c': 3 }

// causes array-callback-return
array.map((val) => {
  doSomething(val)
})

// causes guard-for-in & no-restricted-syntax
for (const key in dict) {
  doSomething(dict[key])
}
</script>

@acjh
Copy link
Author

acjh commented May 24, 2018

The lint does not work when I copied over searchbar.vue and made some random changes. No errors were shown.

I managed to get it working only if exit 0 was removed from the lint script in package.json.

exit 0 was added because of eslint/eslint#7933 on Mac.
I'll add a lintwin script.

The linter seems to be working fine, I managed to fix most styling issues in searchbar.vue, but I am not sure whether we should consider disabling these three rules:

array-callback-return
guard-for-in
no-restricted-syntax

Those should be fixed:

// array.map((val) => {
//   doSomething(val)
// })
array.map(val => doSomething(val));

// for (const key in dict) {
//   doSomething(dict[key])
// }
Object.keys(dict).foreach((key) => {
  doSomething(dict[key]);
});

@yamgent
Copy link
Member

yamgent commented May 24, 2018

I'll add a lintwin script.

Ok, you might want to mention that in README.md.

@acjh acjh merged commit 94e292b into MarkBind:master May 24, 2018
@acjh acjh added this to the v1.1.37-markbind.8 milestone May 24, 2018
@yamgent
Copy link
Member

yamgent commented May 25, 2018

@acjh There is an issue with eslint not being able to parse the *Docs.vue properly.

  • eslint does not recognize content wrapped with <doc-code language="javascript"> as javascript, but parses it as HTML instead.
  • eslint fails to find the import if we reference it from the root directory (e.g. import searchbar from 'src/Searchbar.vue';)

The zip file below contains the offending file (searchbarDocs.vue) and the error generated by eslint:

searchbarDocs.vue.zip

@yamgent
Copy link
Member

yamgent commented May 25, 2018

The part of the source code that is causing the issue:

<doc-code language="javascript">
    new Vue {
      components: {
        searchbar
      },
      data() {
        return {
          searchData: [
            {
              'headings': {
                'normal-include': 'Normal include',
                'establishing-requirements': 'Establishing Requirements',
                'include-segment': 'Include segment',
                'dynamic-include': 'Dynamic include',
                'boilerplate-include': 'Boilerplate include',
                'nested-include': 'Nested include',
                'html-include': 'HTML include',
                'include-from-another-markbind-site': 'Include from another Markbind site',
                'feature-list': 'Feature list',
              },
              'title': 'Hello World',
              'src': 'index.md',
            },
            {
              'headings': {
                'popover-initiated-by-trigger-honor-trigger-attribute':
                'popover initiated by trigger: honor trigger attribute',
                'support-multiple-inclusions-of-a-modal': 'Support multiple inclusions of a modal',
                'remove-extra-space-in-links': 'Remove extra space in links',
              },
              'title': 'Open Bugs',
              'src': 'bugs/index.md',
            },
            {
              'headings': {
                'feature-list': 'Feature list',
              },
              'src': 'sub_site/index.md',
              'title': '',
            },
          ],
          searchTemplate: '<span v-html="item.title | highlight value" /><br />'
            + '<span v-if="item.keywords" v-html="item.keywords | highlightAndExtract value<br />" />'
            + '<span v-if="item.heading" v-html="item.heading.text | highlight value" />',
        };
      }
    }
    </doc-code>
<script>
import searchbar from 'src/Searchbar.vue';

Errors:

C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\vue-strap\docs\example\searchbarDocs.vue
  20:1   error  Expected indentation of 6 spaces but found 4 spaces   vue/html-indent
  22:1   error  Expected indentation of 6 spaces but found 8 spaces   vue/html-indent
  25:1   error  Expected indentation of 6 spaces but found 8 spaces   vue/html-indent
  26:1   error  Expected indentation of 6 spaces but found 10 spaces  vue/html-indent
  27:1   error  Expected indentation of 6 spaces but found 12 spaces  vue/html-indent
  28:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  29:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  30:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  31:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  32:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  33:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  34:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  35:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  36:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  37:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  38:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  39:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  40:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  41:1   error  Expected indentation of 6 spaces but found 12 spaces  vue/html-indent
  42:1   error  Expected indentation of 6 spaces but found 12 spaces  vue/html-indent
  43:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  44:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  45:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  46:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  47:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  48:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  49:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  50:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  51:1   error  Expected indentation of 6 spaces but found 12 spaces  vue/html-indent
  52:1   error  Expected indentation of 6 spaces but found 12 spaces  vue/html-indent
  53:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  54:1   error  Expected indentation of 6 spaces but found 16 spaces  vue/html-indent
  55:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  56:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  57:1   error  Expected indentation of 6 spaces but found 14 spaces  vue/html-indent
  58:1   error  Expected indentation of 6 spaces but found 12 spaces  vue/html-indent
  59:1   error  Expected indentation of 6 spaces but found 10 spaces  vue/html-indent
  60:1   error  Expected indentation of 6 spaces but found 10 spaces  vue/html-indent
  60:34  error  'v-html' directives require that attribute value      vue/valid-v-html
  60:65  error  Parsing error: Unexpected token value                 vue/no-parsing-error
  60:74  error  Disallow self-closing on HTML void elements (<br/>)   vue/html-self-closing
  61:1   error  Expected indentation of 6 spaces but found 12 spaces  vue/html-indent
  61:43  error  'v-html' directives require that attribute value      vue/valid-v-html
  61:43  error  Attribute "html" should be on a new line              vue/max-attributes-per-line
  61:87  error  Parsing error: Unexpected token value                 vue/no-parsing-error
  62:1   error  Expected indentation of 6 spaces but found 12 spaces  vue/html-indent
  62:42  error  'v-html' directives require that attribute value      vue/valid-v-html
  62:42  error  Attribute "html" should be on a new line              vue/max-attributes-per-line
  62:80  error  Parsing error: Unexpected token value                 vue/no-parsing-error
  63:1   error  Expected indentation of 6 spaces but found 8 spaces   vue/html-indent
  65:1   error  Expected indentation of 6 spaces but found 4 spaces   vue/html-indent
  72:23  error  Unable to resolve path to module 'src/Searchbar.vue'  import/no-unresolved

@acjh
Copy link
Author

acjh commented May 25, 2018

It is fine to eslint-disable the specific block/line for now, as @nusjzx has done in #30.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants