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

[New] require-default-props: add option functions #3249

Merged
merged 2 commits into from Apr 27, 2022

Conversation

nix6839
Copy link
Contributor

@nix6839 nix6839 commented Mar 20, 2022

Closes #2396

Todo

  • Update function comments.
  • Add more tests.
  • Update docs.
  • Update changelog.

@nix6839 nix6839 changed the title [New] requireDefaultProps: add option initArgForFnComponents [New] require-default-props: add option initArgForFnComponents Mar 20, 2022
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/util/ast.js Outdated Show resolved Hide resolved
lib/util/ast.js Outdated Show resolved Hide resolved
tests/lib/rules/require-default-props.js Show resolved Hide resolved
@nix6839 nix6839 marked this pull request as ready for review Mar 21, 2022
@nix6839 nix6839 marked this pull request as draft Mar 21, 2022
@nix6839 nix6839 changed the title [New] require-default-props: add option initArgForFnComponents [New] require-default-props: add option assignDefaultToObjectForFc Mar 21, 2022
@nix6839 nix6839 requested a review from ljharb Mar 21, 2022
@nix6839 nix6839 marked this pull request as ready for review Mar 21, 2022
@nix6839
Copy link
Contributor Author

nix6839 commented Mar 21, 2022

Should we also handle forbidDefaultForRequired && assignDefaultToObjectForFc?

Copy link
Member

@ljharb ljharb left a comment

The point of this option is to allow default arguments on props to "count" alongside defaultProps, right? It wouldn't force using default arguments, it would just let them satisfy the requirement.

What about allowDefaultPropArguments? It can't apply except in an SFC, so i don't think that needs calling out, and the = isn't assignment, and it's not "to an object" either.

docs/rules/require-default-props.md Outdated Show resolved Hide resolved
@nix6839 nix6839 force-pushed the init-arg-for-fn-components branch from 7dd399a to 4d6fc2f Compare Mar 22, 2022
@nix6839
Copy link
Contributor Author

nix6839 commented Mar 22, 2022

The point of this option is to allow default arguments on props to "count" alongside defaultProps, right? It wouldn't force using default arguments, it would just let them satisfy the requirement.

What about allowDefaultPropArguments? It can't apply except in an SFC, so i don't think that needs calling out, and the = isn't assignment, and it's not "to an object" either.

No alongside, only use default arguments (this means that forbid to defaultProps). Other than that, I have the same thoughts as you.

Therefore, what about forceDefaultPropArguments, which means to force more allowDefaultPropArguments?

@ljharb
Copy link
Member

ljharb commented Mar 22, 2022

If that’s what it does, then that’s a better name for the option.

@nix6839 nix6839 changed the title [New] require-default-props: add option assignDefaultToObjectForFc [New] require-default-props: add option forceDefaultPropArguments Mar 22, 2022
@nix6839
Copy link
Contributor Author

nix6839 commented Mar 22, 2022

Is there anything I can do?

@nix6839 nix6839 requested a review from ljharb Mar 22, 2022
@nix6839
Copy link
Contributor Author

nix6839 commented Mar 23, 2022

There is an error when ignorePropsValidation and ignoreUnusedPropTypesValidation are false but I don't know what they do.

@nix6839 nix6839 force-pushed the init-arg-for-fn-components branch from 6f141f1 to 2928356 Compare Mar 26, 2022
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/util/ast.js Show resolved Hide resolved
tests/lib/rules/require-default-props.js Outdated Show resolved Hide resolved
tests/lib/rules/require-default-props.js Outdated Show resolved Hide resolved
tests/lib/rules/require-default-props.js Outdated Show resolved Hide resolved
@nix6839 nix6839 changed the title [New] require-default-props: add option forceDefaultPropArguments [New] require-default-props: add option functions Mar 27, 2022
@nix6839 nix6839 requested a review from ljharb Mar 27, 2022
@nix6839 nix6839 force-pushed the init-arg-for-fn-components branch from 9e6b528 to d1a5a40 Compare Apr 3, 2022
@nix6839 nix6839 force-pushed the init-arg-for-fn-components branch from d1a5a40 to 0a12dbd Compare Apr 23, 2022
@codecov
Copy link

codecov bot commented Apr 23, 2022

Codecov Report

Merging #3249 (e3af018) into master (ef733fd) will increase coverage by 0.00%.
The diff coverage is 97.77%.

Current head e3af018 differs from pull request most recent head 24621e4. Consider uploading reports for the commit 24621e4 to get more accurate results

@@           Coverage Diff           @@
##           master    #3249   +/-   ##
=======================================
  Coverage   97.70%   97.71%           
=======================================
  Files         123      123           
  Lines        8716     8750   +34     
  Branches     3160     3173   +13     
=======================================
+ Hits         8516     8550   +34     
  Misses        200      200           
Impacted Files Coverage Δ
lib/rules/require-default-props.js 98.46% <97.67%> (-1.54%) ⬇️
lib/util/ast.js 96.39% <100.00%> (+0.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef733fd...24621e4. Read the comment docs.

@nix6839 nix6839 force-pushed the init-arg-for-fn-components branch from 746f558 to 1a449a8 Compare Apr 23, 2022
@nix6839

This comment has been hidden.

@nix6839
Copy link
Contributor Author

nix6839 commented Apr 24, 2022

Two failed coverages appear to need no testing. Should I guard it?

docs/rules/require-default-props.md Outdated Show resolved Hide resolved
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
@nix6839 nix6839 force-pushed the init-arg-for-fn-components branch from 6c39487 to e3af018 Compare Apr 26, 2022
@nix6839 nix6839 requested a review from ljharb Apr 27, 2022
@ljharb ljharb force-pushed the init-arg-for-fn-components branch from e3af018 to 01541ad Compare Apr 27, 2022
ljharb
ljharb approved these changes Apr 27, 2022
@ljharb ljharb force-pushed the init-arg-for-fn-components branch from 01541ad to 24621e4 Compare Apr 27, 2022
@ljharb ljharb merged commit 24621e4 into jsx-eslint:master Apr 27, 2022
243 checks passed
@sheiman-pavlo
Copy link

sheiman-pavlo commented Apr 29, 2022

When we can expect this to be tagged and released?

@ljharb
Copy link
Member

ljharb commented May 18, 2022

v7.30.0 is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants