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

Add arguments-list which is comma seperated #941

Closed
wants to merge 1 commit into from
Closed

Add arguments-list which is comma seperated #941

wants to merge 1 commit into from

Conversation

lukeapage
Copy link
Member

To avoid the popular javascript hacks to convert @arguments into a comma seperated list

@dmcass
Copy link
Contributor

dmcass commented Sep 4, 2012

Fixes #228, which had a similar suggestion to fix.

@lukeapage
Copy link
Member Author

following suggestion from @cloudhead in #939... I originally liked args but arguments-list is, although more verbose, clearer in what it delivers..

In terms of the alternative (a function that converts space seperated to comma seperated) the only use for it I can see if with the ...@rest syntax for getting a list of the remaining arguments.. if that became a feature request like this, what would we do? Auto create a variable @rest-list or add a function and depreciate arguments-list? or would we reject a request along those lines?

@leafo
Copy link

leafo commented Sep 5, 2012

A function for converting the delimiter has a lot more utility. The best example you mentioned, @rest. LESS right now doesn't do a good job at exposing lists as usable types, but I think in the future there will be a collection of list manipulating functions, especially if we wan't to avoid JavaScript evals. This is pretty important for library authors. (See SASS/Compass and its handling of lists). By using a utility function we are going in the right direction, by adding a new implicit variable we are cluttering the syntax. Especially since the names arguments and arguments-list are essentially meaningless at differentiating their use.

I hope this patch does not make it in. It's a hack to get around one specific use case of arguments.

@dmcass
Copy link
Contributor

dmcass commented Sep 5, 2012

@agatronic yeah I was just linking the PR to the issue it solves. Not trying to change the keyword. I had the same thought about @rest... and how it's likely a follow-up to solving this problem.

@leafo I agree that a utility function to convert something like @arguments to a comma delimited value may be a better approach so that we can deal with any future issues like this in a cleaner way, but we have to be careful about how it's implemented since the list type you're talking about really doesn't exist in LESS at all. Whether or not it should is a different discussion.

@matthew-dean
Copy link
Member

Here's my main problem with this: it actually reinforces a design flaw in LESS, rather than solve the actual problem.

What's the problem: that people aren't actually intending to send multiple arguments to the mixin, and are, in fact, really intending to send a single argument. It's just that that single argument is a legal CSS value that contains a comma.

Here, I'll prove it:

.mymixin(@boxShadow, @color) {
  box-shadow: @boxShadow;
  color: @color;
}

Box shadow is a single argument, but that argument may contain commas. It's that simple, and the problem has been in LESS for a long time. (See Issue #35). String escaping is a hack to solve it. JavaScript regex is a hack. And now @arguments-list is a hack. But none of them address the core problem, which is the design oversight of mixin arguments that contain commas (and this most recent hack won't actually fix the issue, such as in the example above).

I think if we put this in, we risk muddying the realm of best practice for LESS, because it's another hack and we haven't quite addressed what to do about commas in mixins.

If you look at issue #35, there's a discussion about what to do about the problem. There's a few suggestions on the table: replacing commas with semi-colons for mixins, and allowing optional "wrapping" of arguments with parentheses (as SASS does).

I recommend we solve this core issue ASAP, which will effectively wipe the need for these other hacks and temporary shims and new keywords off the table, along with further alleviating the need for JavaScript inside LESS.

@lukeapage
Copy link
Member Author

I see your point...

I like brackets around all the arguments.. I don't like ; for argument delimitting

Can we agree a best approach ASAP and I'll close this and implement what we decide.

@lukeapage
Copy link
Member Author

See revised comment on #35 where @MatthewDL makes some more good points.

@leafo
Copy link

leafo commented Sep 5, 2012

Ideally we would do semicolons for argument delimiter, but it's too late to change that now.

I'm in favor of adding the ability to wrap a list in parentheses.

Edit: I also still think we need functions to manipulate lists, especially if we are making lists first class types.

@lukeapage
Copy link
Member Author

@MatthewDL we have semi-colons as seperators.. you mentioned at once point we could always take both solutions.. do you want this pull request in or shall we close it?

I don't particularly mind either way.

@matthew-dean
Copy link
Member

I say don't put @arguments-list in. While it may, on the surface, seem similar to @arguments, the use case is completely different. With @arguments, I'm probably defining many parts of a shorthand CSS, and then concatenating them in a final value. With @arguments-list, you're not defining many values, it's really only one value that is (was) getting misinterpreted by LESS as a separate value, so then you were reassembling the original value, essentially replacing the comma that was inaccurately removed. It's a hack for a design flaw. Semi-colons in your argument list solves that exact same use case, but in a more accurate and elegant way that's easier to understand.

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.

4 participants