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

Remove obsolete check for rest args after optionals #981

Merged
merged 1 commit into from
Mar 25, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Mar 23, 2015

Fixes #980

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 80.2% when pulling d87a792 on mgreter:bugfix/issue_980 into 948854a on sass:master.

mgreter added a commit that referenced this pull request Mar 25, 2015
Remove obsolete check for rest args after optionals
@mgreter mgreter merged commit f0475ee into sass:master Mar 25, 2015
@@ -1667,9 +1667,6 @@ namespace Sass {
if (has_rest_parameter_) {
error("functions and mixins cannot have more than one variable-length parameter", p->pstate());
}
if (has_optional_parameters_) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? I believe this is still a valid error.\

The error case mentioned in the issue is

[when] optional parameter followed by a variable-length parameter

This change appears to affect the case

[when] a variable-length parameter is followed an optional parameter

Which is a legal scenario.

If this fixes the reported issues I believe the bug is due to something before this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it should be spot on now. The test I removed is obviously wrong, since rest parameters can come after a parameter with optional (default) value (as you can see has_optional_parameters_ is set when we had a preceding default_value. Or can you come up with a sample that doesn't behave correctly?

@mgreter mgreter deleted the bugfix/issue_980 branch April 6, 2015 17:13
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.

Unable to append variable-length parameters after optional parameter
3 participants