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

Parametric mixins: parameters don't match error #2646

Merged

Conversation

SomMeri
Copy link
Member

@SomMeri SomMeri commented Jul 22, 2015

Named argument in mixin call counted against requires argument with different name in mixin definition. #2645

@seven-phases-max
Copy link
Member

Thanks! The only remark (sorry, as my usual :) I was just looking at the code related to this issue myself and noticed that for it to be fixed (in easy way) the error trigger at https://github.com/less/less.js/blob/master/lib/less/tree/mixin-definition.js#L99 has to be eliminated somehow...
Now since this change actually makes that error code unreachable (or at least I can't invent an example where it could now) - would not we want to eliminate that never used code too?

On the other hand it never harms to keep it "just in case" (so I'm actually only concerning of it to be misleading for further changes). Any ideas?

@SomMeri
Copy link
Member Author

SomMeri commented Jul 23, 2015

@seven-phases-max I sometimes put equivalent of "Bug: unreachable code reached please open new issue" into code. Theoretically, it could help with early error detection. Practically, I seen such report in tracker maybe once, but it helped find errors when refactoring a few times. It also helped to remember that branch should not be reachable few months later :)

lukeapage added a commit that referenced this pull request Sep 17, 2015
…ameters-2645

Parametric mixins: parameters don't match error
@lukeapage lukeapage merged commit 8dc3bfb into less:master Sep 17, 2015
mishal added a commit to mishal/iless that referenced this pull request Sep 18, 2015
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.

3 participants