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

Support maps in list context (index, nth, etc) #1930

Closed
madlandproject opened this issue Feb 25, 2016 · 7 comments
Closed

Support maps in list context (index, nth, etc) #1930

madlandproject opened this issue Feb 25, 2016 · 7 comments

Comments

@madlandproject
Copy link

madlandproject commented Feb 25, 2016

Example file, sasstest.scss :

$test-map : (
        mykey    : 10px,
        otherKey : 20px,
        moarKey  : 30px
);


@mixin testMixin($key-name) {

  // get index in map
  $item-value : map-get($test-map, $key-name);
  $item-index : index($test-map, ($key-name $item-value) );

  @debug $item-index;

}

@include testMixin(otherKey);

Expected output (using ruby sass 3.4.21) :
sasstest.scss:14 DEBUG: 2

Output with libsass :
sasstest.scss:14 DEBUG: null

@mgreter
Copy link
Contributor

mgreter commented Feb 26, 2016

I do see the difference, but wonder if this is actually indended behavior. IMHO this is quite a "funky" feature so to speak (never seen any similar "index" function with "hash-maps") //CC @chriseppstein

$test-map : (
  key : 20px,
);

foo {
  bar: index($test-map, ("key" 20px) );
  baz: index($test-map, ("key", 20px) );
  ibar: inspect(index($test-map, ("key" 20px) )); // 1
  ibaz: inspect(index($test-map, ("key", 20px) )); // null
}

Yields in ruby sass:

foo {
  bar: 1;
  ibar: 1;
  ibaz: null; }

@xzyfer
Copy link
Contributor

xzyfer commented Feb 26, 2016

For context, when a map is used with index (instead of a list) it's treated a list of lists. i.e. (foo: bar, bar: baz), is (foo bar, bar baz).

This applies for all list functions. This is documented in the Sass changelog for 3.3.

All the existing list functions also work on maps, treating them as lists of pairs. For example, nth((foo: 1, bar: 2), 1) returns foo 1. Maps can also be used with @each, using the new multiple assignment feature (see below):
source

@mgreter
Copy link
Contributor

mgreter commented Apr 6, 2016

Would be great if someone could come up with a complete list, or better spec test suite for all related functions, since this doesn't seem to be related to index only (I don't think nth has that support)!?

@mgreter
Copy link
Contributor

mgreter commented Apr 22, 2016

On another note this should probably be connected to a refactoring with selector lists, since all three types seem to share some common properties (iterable etc). Would probably help we had this covered in AST directly and not having to implement the logic again in each function (nth, inspect, etc).

One of the main issues with lists is that we sometimes have nested lists (specially in regard to functions arguments). Selector_Lists became IMO quite stable during the recent refactorings for selector functions and parent selector handling. But there is also a big potential for further refactorings. And I'm really not sure if the initial design for Complex, Compound, Simple selector was such a great design. I don't mean the types per se, but how it is handled with head and tail as the approach Node class has with a list of simple selectors and combinators seems more natural to me. Not sure where the linked list approach with tail has it's strength (probably visitor pattern), but it certainly is not manipulation.

@chriseppstein
Copy link
Contributor

Sass maps are ordered. As such, the pairs within them have an intrinsic index based on insertion order.

@saper
Copy link
Member

saper commented May 1, 2016

Ouch. So we need to implement them as the list of (k,v) items, never the hash map...

@mgreter
Copy link
Contributor

mgreter commented May 1, 2016

We already currently implement them as std::unordered_map, but not sure what performance implications are for indexed access (when changed to ordered map). In the worst case we have to keep an additional vector, but I guess that's pretty much how std::map is implemented in the first place.

Edit: sorry, I though it was an ordered map. I changed that one once, but we can easily go back to an ordered map. We "just" need to implement the indexed access in libsass functions.

Amend: from a short check it seems that we need to add a vector deque to our map object to support fast indexed access. Not super nice to add the memory overhead, but also not very hard to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants