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

Fix duplicate key handling for maps #1195

Merged
merged 1 commit into from
May 12, 2015
Merged

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented May 10, 2015

Deferred check from parser to evaluation
Fixes #1187

@mgreter mgreter self-assigned this May 10, 2015
@mgreter mgreter added this to the 3.2.4 milestone May 10, 2015
@mgreter mgreter force-pushed the bugfix/issue_1187 branch 2 times, most recently from 7d262a8 to 8b556bc Compare May 10, 2015 17:37
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 80.14% when pulling 8b556bc on mgreter:bugfix/issue_1187 into fb82f0b on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented May 11, 2015

👍

@@ -1073,8 +1073,15 @@ namespace Sass {
(*map) << make_pair(key, value);
}

if (map->has_duplicate_key())
{ error("Duplicate key \"" + map->get_duplicate_key()->perform(&to_string) + "\" in map " + map->perform(&to_string) + ".", pstate); }
// Check was moved to eval step
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these comments?

@mgreter
Copy link
Contributor Author

mgreter commented May 12, 2015

Ah, the pstate stuff makes sure the Map object source map is pointing from parenthese to parenthese, since we don't lex it with one call ... Just saw it was off , so I thought I'd fix it while beeing at it ...

@xzyfer
Copy link
Contributor

xzyfer commented May 12, 2015

For some more context on the original error message. It was added to address the issue that we weren't throwing duplicate key issues, I expected we'd eventually need it in eval as well. It was added here to match the Ruby behaviour.

IIRC Ruby validates duplicate keys when adding keys to a Map AST node. However it only validates static values. The ideal way to handle this would be to have the Map AST either throw the error or have it return an int > 0 on failure (only for String_Constant and String_Quoted).

@xzyfer
Copy link
Contributor

xzyfer commented May 12, 2015

Ah right, it's because the close ) is lexed by parse_value ?

Deferred check from parser to evaluation
Fixes sass#1187
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 80.14% when pulling 2eef52a on mgreter:bugfix/issue_1187 into 8ca8816 on sass:master.

mgreter added a commit that referenced this pull request May 12, 2015
Fix duplicate key handling for maps
@mgreter mgreter merged commit f2ed3c0 into sass:master May 12, 2015
@mgreter mgreter deleted the bugfix/issue_1187 branch May 12, 2015 17:51
@xzyfer
Copy link
Contributor

xzyfer commented Dec 28, 2015

Spec activated sass/sass-spec#656

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

Successfully merging this pull request may close these issues.

Weird behaviour with lists as map keys
3 participants