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

feat: fixable match-component-file-name rule #1874

Merged

Conversation

mahaker
Copy link
Contributor

@mahaker mahaker commented Apr 29, 2022

fix #1816

This plugin has always helped me.
Thank you very much.

There are two fixes.

  1. Add 'Literal#raw' property
  2. fixable match-component-file-name rule

vue-eslint-parser's ESLintLiteralBase Node has 'raw' property.
ref: https://github.com/vuejs/vue-eslint-parser/blob/160f4efc4eaf363662b464a4a26a4c9e514deb5d/src/ast/nodes.ts#L395

It was necessary to identify quotes in the match-component-file-name's fixer.
@@ -102,19 +102,25 @@ module.exports = {
*/
function verifyName(node) {
let name
let quote = ''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defining let quote, resulted in a typescript error.

Variable 'quote' implicitly has type 'any' in some locations where its type cannot be determined.

Copy link
Member

Choose a reason for hiding this comment

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

You could instead set a type without initializing the variable:

/** @type {string} */
let quote

// equivalent to the following Typescript syntax:
let quote: string

Copy link
Member

Choose a reason for hiding this comment

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

But maybe it would make sense to only compute the quote in the fixer function:

fix(fixer) {
  const quote = node.type === 'TemplateLiteral' ? '`' : node.raw[0]
  return fixer.replaceText(node, `${quote}${filename}${quote}`)
}

Then you don't have to initialize it and the meaning of the variable is immediately clear from the context.

@@ -347,6 +347,7 @@ export interface PrivateIdentifier extends HasParentNode {
export interface Literal extends HasParentNode {
type: 'Literal'
value: string | boolean | null | number | RegExp | BigInt
raw: string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

console.log(node), the following object can be displayed.
The fixer use the raw property to determine the single/doublequotes.

<ref *2> Node {
  type: 'Literal',
  start: 38,
  end: 51,
  loc: SourceLocation {
    start: { line: 3, column: 10 },
    end: { line: 3, column: 23 }
  },
  range: [ 38, 51 ],
  value: 'MyComponent',
  raw: "'MyComponent'",
  parent: <ref *1> Node {
    type: 'Property',
    start: 32,
    end: 51,
    loc: SourceLocation { start: [Object], end: [Object] },
    range: [ 32, 51 ],
    method: false,
    shorthand: false,
    computed: false,
    key: Node {
      type: 'Identifier',
      start: 32,
      end: 36,
      loc: [SourceLocation],
      range: [Array],
      name: 'name',
      parent: [Circular *1]
    },
    value: [Circular *2],
    kind: 'init',
    parent: Node {
      type: 'ObjectExpression',
      start: 26,
      end: 110,
      loc: [SourceLocation],
      range: [Array],
      properties: [Array],
      parent: [Node]
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found Literal's raw property is defined here.

Comment on lines 121 to 123
fix(fixer) {
return fixer.replaceText(node, `${quote}${filename}${quote}`)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this should not be an auto-fix, since it's sometimes the case that the filename is wrong and the name property is correct.

Instead, please change this to be a suggestion instead. Then cases where the filename is correct and the name property is wrong can be easily corrected in the editor, without causing wrong fixes when running from the commandline.

The suggestion description should be "Rename component to match file name.", as described in the issue #1816.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much. I understand the reason to using suggest instead of fix.

@@ -102,19 +102,25 @@ module.exports = {
*/
function verifyName(node) {
let name
let quote = ''
Copy link
Member

Choose a reason for hiding this comment

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

You could instead set a type without initializing the variable:

/** @type {string} */
let quote

// equivalent to the following Typescript syntax:
let quote: string

@@ -102,19 +102,25 @@ module.exports = {
*/
function verifyName(node) {
let name
let quote = ''
Copy link
Member

Choose a reason for hiding this comment

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

But maybe it would make sense to only compute the quote in the fixer function:

fix(fixer) {
  const quote = node.type === 'TemplateLiteral' ? '`' : node.raw[0]
  return fixer.replaceText(node, `${quote}${filename}${quote}`)
}

Then you don't have to initialize it and the meaning of the variable is immediately clear from the context.

@FloEdelmann
Copy link
Member

Thanks for your first contribution! I have a few comments, see above. After you're done, please run npm run update to update the docs (which adds a hint that the rule has suggestions).

@mahaker
Copy link
Contributor Author

mahaker commented May 1, 2022

Very thanks for the review!!

I will confirm comments, and fix them.
Also run npm run update to update the docs.

@mahaker mahaker force-pushed the fixable-match-component-file-name-rule branch from 2bd2cb4 to 0a9a584 Compare May 1, 2022 06:14
@mahaker
Copy link
Contributor Author

mahaker commented May 1, 2022

I force pushed commit. Could you please review it?
If the review is ok, i will run npm run update to update the docs.

I have checked the operation of suggest with my VSCode and ESLint extension.
It seems to be working correctly.

1

image

2: Quick Fix clicked

image

3: Rename component to... clicked

image

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!
Almost LGTM! I have only one request to change the test script.

}
]
],
output: null
Copy link
Member

Choose a reason for hiding this comment

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

Remove output as it is no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very thanks for the review!!

I removed output, and run update the document.

improve vuejs#1816

For the following reasons why name option changed.
If change file name, alto need to change import statement.

Also, this rule not auto-fixed, because the file name may be incorrect.
@mahaker mahaker force-pushed the fixable-match-component-file-name-rule branch from 0a9a584 to 4ff89bb Compare May 4, 2022 02:27
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@FloEdelmann FloEdelmann requested a review from ota-meshi May 7, 2022 15:13
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! thank you!

@ota-meshi ota-meshi merged commit 4daf4c8 into vuejs:master May 9, 2022
@mahaker mahaker deleted the fixable-match-component-file-name-rule branch May 9, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add suggestion(s) to vue/match-component-file-name
3 participants