-
-
Notifications
You must be signed in to change notification settings - Fork 360
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
Add prefer-default-parameters
rule
#632
Add prefer-default-parameters
rule
#632
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay.
Very good format and clear logic. ❤️
Can you update code with code in function body to multiple lines? Should easier to read.
One more thing, since the fix is not safe , we should use suggestion api, but don't worry, we can easily convert it. example |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
We still need to check all of them to see if it's actually one of the parameters, but I chose to skip the fixing if it's not the last parameter.
Done. I kept the formatting of some of the tests to make sure that the fixer correctly formats one-line functions.
I've noticed that there are more cases where a fix would actually break the code, so I chose to use the suggestions API exclusively: function abc(foo) {
foo += 'bar';
foo = foo || 'bar';
} |
I think we can simple this logic, only report one problem each time on one function. but we need consider following cases
|
Another edge case, since the assignment can be used as value. function foo(a){
doSomething(
a = a || 1
)
doAnotherThing(a)
} or function foo(a){
const b = a = a || 1
} So, we should choose leave My suggestion, alway leave |
@medusalix What do you think the suggestion above? |
@fisker Sorry for the late reply, I'm pretty busy right now, I'll take a look at your suggestions when I get the time. |
Yeah, that's already checked by the selector: |
|
I moved one of your test cases because it can actually be fixed. References are now checked using |
Good, you checked the |
I think the problem we can't resolve here is function abc(foo) {
foo = foo || 'bar';
console.log(foo);
} VS function abc(foo) {
console.log(foo);
foo = foo || 'bar';
} function abc(foo) {
useFoo();
foo = foo || 'bar';
function useFoo() {
console.log(foo);
}
} VS function abc(foo) {
notUseFoo();
foo = foo || 'bar';
function notUseFoo() {
}
} |
Bump |
I think I've found a pretty good solution for the first point @fisker outlined above:
foo = foo || 123; // Reference 1 and 2
console.log(foo); // Reference 3
// First reference is the default-assignment; invalid, can be fixed. console.log(foo); // Reference 1
foo = foo || 123; // Reference 2 and 3
// First reference is the console.log statement; valid.
const bar = foo || 'bar'; // Reference 1
console.log(foo, bar); // Reference 2
// More than one reference; valid. const bar = foo || 'bar'; // Reference 1
console.log(bar);
// Only one reference; invalid, can be fixed. |
I've done some tests with the function abc(foo) {
let bar = true;
bar = false;
foo = foo || 123;
console.log(foo);
} This example would be ignored by the rule because @fisker I think ignoring cases where there is any |
I'm running out of ideas. 😄 |
@fisker I've implemented the |
// @fisker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot the review progress, I read all comments from beginning again, seems good enough, we can still improve it if any issue found.
Some notes: I've tried to keep the rule simple by only reporting
literal
assignments (see this comment). Combining parameters with e.g. a return value of another function or variable might be desired in some cases and should in my opinion not be reported. I've had to work around arrow functions with only one parameter and no parentheses (foo => {}
) to make sure the default parameter gets inserted correctly by including the missing parentheses.Fixes #29
IssueHunt Summary
Referenced issues
This pull request has been submitted to:
IssueHunt has been backed by the following sponsors. Become a sponsor