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

[Fix] boolean-prop-naming: add check for typescript "boolean" type #2930

Merged
merged 1 commit into from Feb 21, 2021

Conversation

@vedadeepta
Copy link
Contributor

@vedadeepta vedadeepta commented Feb 21, 2021

  • Fixes #2892
  • Includes test

Overview of change:

Add a check for typescript boolean type without the need to explicitly define react PropTypes.

type Props = {
  enabled: boolean
}
const Hello = (props: Props) => <div />;

Will throw a linting error for the above code.

@ljharb
ljharb approved these changes Feb 21, 2021
@ljharb ljharb force-pushed the vedadeepta:2892-boolean-prop-naming branch from b8d3d5c to ae5ace5 Feb 21, 2021
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 21, 2021

(note, the new tests weren't actually running since [].concat wasn't being used; i've fixed that)

@ljharb ljharb changed the title [fix] boolean-prop-naming: add check for typescript "boolean" type [Fix] boolean-prop-naming: add check for typescript "boolean" type Feb 21, 2021
@vedadeepta
Copy link
Contributor Author

@vedadeepta vedadeepta commented Feb 21, 2021

(note, the new tests weren't actually running since [].concat wasn't being used; i've fixed that)

that's weird, because it did run locally using this

node node_modules/mocha/bin/_mocha tests/lib/rules/boolean-prop-naming.js

@ljharb ljharb merged commit ae5ace5 into yannickcr:master Feb 21, 2021
43 checks passed
43 checks passed
@github-actions
Automatic Rebase
Details
@github-actions
Require “Allow Edits”
Details
@github-actions
matrix
Details
@github-actions
pretest
Details
@github-actions
readme
Details
@github-actions
latest majors (15, 7)
Details
@github-actions
posttest
Details
@github-actions
latest majors (15, 6)
Details
@github-actions
latest majors (15, 5)
Details
@github-actions
latest majors (15, 4)
Details
@github-actions
latest majors (14, 7)
Details
@github-actions
latest majors (14, 6)
Details
@github-actions
latest majors (14, 5)
Details
@github-actions
latest majors (14, 4)
Details
@github-actions
latest majors (13, 7)
Details
@github-actions
latest majors (13, 6)
Details
@github-actions
latest majors (13, 5)
Details
@github-actions
latest majors (13, 4)
Details
@github-actions
latest majors (12, 7)
Details
@github-actions
latest majors (12, 6)
Details
@github-actions
latest majors (12, 5)
Details
@github-actions
latest majors (12, 4)
Details
@github-actions
latest majors (11, 7)
Details
@github-actions
latest majors (11, 6)
Details
@github-actions
latest majors (11, 5)
Details
@github-actions
latest majors (11, 4)
Details
@github-actions
latest majors (10, 7)
Details
@github-actions
latest majors (10, 6)
Details
@github-actions
latest majors (10, 5)
Details
@github-actions
latest majors (10, 4)
Details
@github-actions
latest majors (9, 6)
Details
@github-actions
latest majors (9, 5)
Details
@github-actions
latest majors (9, 4)
Details
@github-actions
latest majors (8, 6)
Details
@github-actions
latest majors (8, 5)
Details
@github-actions
latest majors (8, 4)
Details
@github-actions
latest majors (7, 5)
Details
@github-actions
latest majors (7, 4)
Details
@github-actions
latest majors (6, 5)
Details
@github-actions
latest majors (6, 4)
Details
@github-actions
latest majors (5, 4)
Details
@github-actions
latest majors (4, 4)
Details
@github-actions
node 4+
Details
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 21, 2021

@vedadeepta the tests completed, but weren't actually running, because parsers.TS() returns an array, and the tests are objects.

@vedadeepta
Copy link
Contributor Author

@vedadeepta vedadeepta commented Feb 22, 2021

cropped

This is the output i get when i run the tests without concat on this branch using the command node node_modules/mocha/bin/_mocha tests/lib/rules/boolean-prop-naming.js (I'm using parser.TS() two times separately for the last two invalid tests e.g parser.TS(testObj1), parser.TS(testObj2) instead of parser.TS(testObj1, testObj2)). Should it be something else?

Also i don't think the last test on the master branch for boolean-prop-naming is running at all. Change it to something like this

parsers.TS({
    code: `
    type TestConstType = {
      enabled: boolean
    }
    const HelloNew = (props: TestConstType) => { return <div /> };
    `,
    options: [{
      rule: '^is[A-Z]([A-Za-z0-9]?)+'
    }],
    parser: parsers['@TYPESCRIPT_ESLINT'],
    errors: [{
      message: 'Prop name (enabled) doesn\'t match rule (^is[A-Z]([A-Za-z0-9]?)+)'
    }]
  }, {
    code: `
    type TestFNType = {
      isEnabled: boolean
    }
    const HelloNew = (props: TestFNType) => { return <div /> };
    `,
    options: [{
      rule: '^is[A-Z]([A-Za-z0-9]?)+'
    }],
    parser: parsers['@TYPESCRIPT_ESLINT'],
    errors: [{
      message: 'Prop name (enabled) doesn\'t match rule (^is[A-Z]([A-Za-z0-9]?)+)'
    }]
  })

The last one should fail but it does not. It also shows "61 passing" (+ no failures reported) instead of 62 as in the screenshot above. The last test is completely omitted on master.

I think there is something wrong with parser.TS function. it just returns the first object most probably.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 23, 2021

parsers.TS returns either its first argument (which is meant to be an array), or an array.

I'll make some changes so it's harder to misuse.

ljharb added a commit to ljharb/eslint-plugin-react that referenced this pull request Feb 23, 2021
@vedadeepta
Copy link
Contributor Author

@vedadeepta vedadeepta commented Feb 23, 2021

Thanks for looking into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants