Default params & duplicate param arguments

[lukeapage]

Sorry, 2 more fixes in one because I needed a refactor from one for the other.

Change default argument processing to error if you attempt to access a
future argument or the argument you are assigning yourself.
Fixes #2422

Fix the behaviour of params to respect shadow inner or outer, which causes
a warning and strict mode, which causes an error.
Fixes #2492

[lukeapage]

Added fix for #2494

[jugglinmike]

The fix for duplicate parameter names does not consider the strictness of the function itself. For instance, the following does not generate an error with this patch applied:

// jshint shadow: true
function f(a, a) {
  'use strict';
}

This is not a regression, though, so I'd be okay with tackling that problem in a follow-up patch. How would you like to handle it, @lukeapage?

[lukeapage]

I fixed both comments.. not sure how the squashing should go though since I have two commits and it related to the first one?

[lukeapage]

not sure how the squashing should go though since I have two commits and it related to the first one?

re-ordered them, didn't realise you could do it :)

[lukeapage]

rebased on master. Changes around how parameters are added in the catch case.

[jugglinmike]

Hey Luke, thanks for addressing my feedback! There are a few things that I
think we could improve about this latest approach:

• Method naming: The in-line comment explains the inStrictMode method's
  purpose is to enforce restrictions on strict-mode function parameters. The
  name itself doesn't quite convey that
• Member variables: this introduces a new property, "(unraisedStrictWarnings)",
  in kind of an ad-hoc way. For readability/traceability, tt would be nice if
  this were more formally initialized as part of the scope creation logic.
• Duplication: There are two distinct branches that concern the generation of
  error E011. I can see why this needs to be so in the current factoring, but
  ideally we could avoid it altogether
• Extraneous warnings. If JSHint produces an error because of a syntax error,
  it's not really necessary/helpful to also produce a warning due to shadowing.

I've made a patch for your patch here. It addresses some of these concerns directly, while refactoring others away. What do you think?

By the way: I know this feedback is delayed, but I usually don't feel
comfortable making such substantive critique without having some confidence in
the feasibility of an alternative. This often leads me to do a fair amount of
experimentation on my own, and in this case it produced the patch above. This
process necessarily slows down the review process (harder to find the time to
do this kind of research/experimentation), but my hope is that it's worthwhile
for contributors both because it yields more realistic feedback and because it
demonstrates my willingness to help out. Your contributions-in-need-of-review
have been piling up these past few weeks, though, so I'd like to check in with
you about this. If you'd prefer quicker review turnaround at the risk of less
directed (read: possibly inane) feedback, then I'm happy to try that
style--please let me know!

[lukeapage]

Thanks, your patch is much better. I think there are a couple of easy-to-fix problems with it (also your link doesn't work, I think you used the pull request format rather than the commit one?)

Do you want to make a new PR with both our commits in or? what do you want to do?

[jugglinmike]

Sorry about that! It sounds like you found it, but just in case:

jugglinmike@72a4464

I'm happy to keep this branch/pull request if you don't mind applying that patch and pushing it up.

$ curl https://github.com/jugglinmike/jshint/commit/72a4464287b1f3e01cb440e8e1894dbed72dab1f.patch | git am
$ git push origin default-params

[jugglinmike]

Oh, just now seeing your comments on that commit. Hm, maybe I should incorporate your feedback and make a new commit from that. That's not what I suggested above, so I'll wait to hear back from you before doing anything.

[lukeapage]

By the way: I know this feedback is delayed, but I usually don't feel
comfortable making such substantive critique without having some confidence in
the feasibility of an alternative. This often leads me to do a fair amount of
experimentation on my own, and in this case it produced the patch above. This
process necessarily slows down the review process (harder to find the time to
do this kind of research/experimentation), but my hope is that it's worthwhile
for contributors both because it yields more realistic feedback and because it
demonstrates my willingness to help out. Your contributions-in-need-of-review
have been piling up these past few weeks, though, so I'd like to check in with
you about this. If you'd prefer quicker review turnaround at the risk of less
directed (read: possibly inane) feedback, then I'm happy to try that
style--please let me know!

I think quicker feedback even if inane is beneficial for any open source project. If people raise a PR and don't get feedback they assume it will sit there forever (my colleague contributed to jquery ui a year and half ago and there still hasn't been any negative feedback as to why it isn't being pulled) and that there was no point in raising the PR. In a way I think I matter less than other contributors because I already have time invested and I know you are slowly working through them (though about a minute before I got this response I was wondering if you'd missed the PR's because I made so many of them). That's why I've tried to be quick to review @nicolo-ribaudo code, because I get the feeling, I could be completely wrong, that if he has alot of PR's open, he stops contributing) - hopefully I've shown the balance of leaving the ones I need you to look at and merging the simpler ones. But its possible I'm wrong and that more inane, terse feedback might put people off more than no response.

In this particular PR, If you had just come back with those 4 short comments I'm not sure whether I'd have got to the same solution as you or not - I definitely missed it the first time round, but I might have got it if challenged. My style is probably closer to inane/quick feedback on PR's, merging those that are simple asap and then go back and help people on PR's that possibly non-helpful comments were left on, if they haven't in that time figured out a better solution to look at. But its about what you enjoy as well, so, I personally really don't mind.

[lukeapage]

Oh, just now seeing your comments on that commit. Hm, maybe I should incorporate your feedback and make a new commit from that.

thats fine, I'll await the next version. am also happy to push to a jshint branch and create a new PR so you can push directly to it.

[jugglinmike]

I've pushed up a commit addressing some of your feedback. I don't believe a change is necessary for your other comment, though. I may be mistaken, so please let me know.

I'm working from the following branch: https://github.com/jugglinmike/jshint/tree/default-params-improvement . The comparison view with your branch for this pull request is here: lukeapage/jshint@default-params...jugglinmike:default-params-improvement

[lukeapage]

I think your commits look good - and I think you've addressed all of your own feedback? So I'm happy with this if you are. Let me know if there is anything else to do. I've put your commits onto this PR.

[jugglinmike]

Okay! I'm going to try to clean up the history a bit; here's my plan:

• Break the "code review comments" into pieces and squash the changes into the appropriate commit that precedes it
• Change the "[[FIX]]" in my first commit to "[[CHORE]]"
• Squash my second commit into my first

...after all that's done, I'll land it in master with a merge commit. I'll keep you posted!

[jugglinmike]

Teamwork!