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

Parser should catch error on duplicate keys in maps #628

Closed
lunelson opened this issue Nov 6, 2014 · 3 comments · Fixed by #768
Closed

Parser should catch error on duplicate keys in maps #628

lunelson opened this issue Nov 6, 2014 · 3 comments · Fixed by #768

Comments

@lunelson
Copy link

lunelson commented Nov 6, 2014

So, ruby-sass throws an error when a map is declared that has any duplicate keys, including any at nested levels. i.e. maps that are values or keys within other maps:

$map: (
  alpha: 1,
  beta: 2,
  gamma: 3,
  delta: (
    eta: 5,
    eta: 6,
  ),
);
Duplicate key "eta" in map (eta: 5, eta: 6).

Libsass currently does not run this check, and should; however, libsass can also do better than ruby-sass here. Note that ruby-sass fails to pick up duplicate-key maps that result from other operations. Example:

$map1: (
  alpha: 1,
  beta: 2,
  gamma: 3,
);

$map2: (
  alpha: 2
);

$map3: join($map1, $map2);

.test {
  out: inspect($map3);
}
.test {
  out: alpha 1, beta 2, gamma 3, alpha 2;
}

So imo libsass should have a map-validity check routine that runs on anything that involves SassScript maps or lists (as the latter may contain a map) as outputs or assignments.

@lunelson lunelson changed the title Make SassScript Map parsing throw error on duplicate keys Parser should catch error on duplicate keys in maps Nov 6, 2014
@xzyfer
Copy link
Contributor

xzyfer commented Nov 6, 2014

I thought this had been addressed previously. My bad. Good catch.

however, libsass can also do better than ruby-sass

I like the idea, but as far as Libsass is concerned Ruby sass is the "right way". Our goal is to act exactly as they do. If you want this feature in Libsass I would the suggestion on the Ruby sass repo.

@lunelson
Copy link
Author

lunelson commented Nov 7, 2014

Right. Good idea, I will put up an issue on ruby-sass about it too.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 16, 2014

We can't currently test this case. See sass/sass-spec#136

saper added a commit to saper/sass-spec that referenced this issue Sep 19, 2015
saper added a commit to saper/sass-spec that referenced this issue Sep 20, 2015
saper added a commit to saper/sass-spec that referenced this issue Sep 20, 2015
saper added a commit to saper/sass-spec that referenced this issue Sep 21, 2015
Still marked as "todo" due to libsass
message mismatch.

Test for sass/libsass#628
Fixes sass#133
saper added a commit to saper/sass-spec that referenced this issue Sep 21, 2015
saper added a commit to saper/sass-spec that referenced this issue Sep 21, 2015
mgreter added a commit to mgreter/sass-spec that referenced this issue Apr 22, 2016
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.

3 participants