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

Make if() a builtin function so that only one of the two outcomes are evaluated. #836

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@chriseppstein
Member

chriseppstein commented Jul 3, 2013

It's super annoying, that it doesn't work like a proper if.

This addresses #470.

chriseppstein added some commits Jul 3, 2013

Preserve the authored format of keyword arguments during conversion.
Change-Id: I2770f132f790b36e65ec8e18d36f4c343868459e
Make if() a builtin function so that only one of the two outcomes are…
… evaluated.

Change-Id: Idca032fc06eacafdea34953f568819abae34f806
@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Jul 3, 2013

Member

addressed my own code comments. ready for your review @nex3.

Member

chriseppstein commented Jul 3, 2013

addressed my own code comments. ready for your review @nex3.

Fix ruby 1.9 issue with DelegateClass
Change-Id: Id10b8bb08f503142c3a310bd2299ef59ed9bcf07
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jul 11, 2013

Contributor

This would definitely be a very cool fix. I've been bothered by this behaviour a couple of days ago; had to wrap the whole thing into a conditional @if statement. Not great.

Can we expect this patch for v3.3 maybe? Or even before?

Contributor

HugoGiraudel commented Jul 11, 2013

This would definitely be a very cool fix. I've been bothered by this behaviour a couple of days ago; had to wrap the whole thing into a conditional @if statement. Not great.

Can we expect this patch for v3.3 maybe? Or even before?

# to the original keys that were stored. If several different values normalize
# to the same value, whichever is stored last wins.
class NormalizedMap < DelegateClass(Hash)

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

Style nit: extra newline.

@nex3

nex3 Jul 26, 2013

Contributor

Style nit: extra newline.

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

Removed.

@chriseppstein
# A hash that normalizes its string keys while still allowing you to get back
# to the original keys that were stored. If several different values normalize
# to the same value, whichever is stored last wins.
class NormalizedMap < DelegateClass(Hash)

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

This'll need to be integrated with OrderedHash in the maps branch somehow. Probably just DelegateClass(ruby1_8? OrderedHash : Hash).

@nex3

nex3 Jul 26, 2013

Contributor

This'll need to be integrated with OrderedHash in the maps branch somehow. Probably just DelegateClass(ruby1_8? OrderedHash : Hash).

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

I am going to land this commit on master and then merge it into the maps branch where I will address this.

@chriseppstein

chriseppstein Jul 30, 2013

Member

I am going to land this commit on master and then merge it into the maps branch where I will address this.

# We delegate all hash methods that are not overridden here to @map.
super(@map)
hash.each {|k, v| self[k] = v } if hash

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

The native Hash will only do this if its argument is itself a Hash. Otherwise it'll just ignore the argument. This should do the same. Alternately, since we never actually construct this with arguments, we could just only support the 0-arg constructor.

Style nit: no space before }.

@nex3

nex3 Jul 26, 2013

Contributor

The native Hash will only do this if its argument is itself a Hash. Otherwise it'll just ignore the argument. This should do the same. Alternately, since we never actually construct this with arguments, we could just only support the 0-arg constructor.

Style nit: no space before }.

#
# This can be overridden to create other normalization behaviors
def normalize(key)
key.tr("-","_")

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

It's a small thing, but it feels like the underlying standard should be dashes, since that's the suggested style.

Style nit: space after ,.

@nex3

nex3 Jul 26, 2013

Contributor

It's a small thing, but it feels like the underlying standard should be dashes, since that's the suggested style.

Style nit: space after ,.

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

While dashes are the preferred style in Sass files, our preferred notation for implementation details has been underscores because it integrates more easily with ruby names. I am simply maintaining that convention.

Space added.

@chriseppstein

chriseppstein Jul 30, 2013

Member

While dashes are the preferred style in Sass files, our preferred notation for implementation details has been underscores because it integrates more easily with ruby names. I am simply maintaining that convention.

Space added.

@@ -29,8 +29,8 @@ class Funcall < Node
# @param name [String] See \{#name}
# @param args [Array<Node>] See \{#args}
# @param keywords [Sass::Util::NormalizedMap<{String => Node}>] See \{#keywords}

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

Sass::Util::NormalizedMap<Node>, since the key is always a string for NormalizedMap.

@nex3

nex3 Jul 26, 2013

Contributor

Sass::Util::NormalizedMap<Node>, since the key is always a string for NormalizedMap.

# Returns the hash with the keys as they were stored (before normalization)
def as_stored
Sass::Util.map_keys(@map) {|k| @key_strings[k] }

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

Style nit: no space before }.

@nex3

nex3 Jul 26, 2013

Contributor

Style nit: no space before }.

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

removed.

@chriseppstein
# Specifies how to transform the key.
#
# This can be overridden to create other normalization behaviors

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

Nit: period

@nex3

nex3 Jul 26, 2013

Contributor

Nit: period

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

added.

@chriseppstein
super(normalized)
end
# Returns the hash with the keys as they were stored (before normalization)

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

Nit: period

Also, return type documentation.

@nex3

nex3 Jul 26, 2013

Contributor

Nit: period

Also, return type documentation.

end
# @private
def deep_copy

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

I'm not a fan of defining #deep_copy on this just because it happens to be a class we control. I feel like #deep_copy should only be defined on objects that are part of Sass proper, whereas this is more of a generic utility object. I also don't like that we need to do respond_to? to make it work in general.

@nex3

nex3 Jul 26, 2013

Contributor

I'm not a fan of defining #deep_copy on this just because it happens to be a class we control. I feel like #deep_copy should only be defined on objects that are part of Sass proper, whereas this is more of a generic utility object. I also don't like that we need to do respond_to? to make it work in general.

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

Agree. I've removed it and the deep copy is handled in the Funcall now:

copied_keywords = Sass::Util::NormalizedMap.new
@keywords.as_stored.each {|k,v| copied_keywords[k] = v.deep_copy}
@chriseppstein

chriseppstein Jul 30, 2013

Member

Agree. I've removed it and the deep copy is handled in the Funcall now:

copied_keywords = Sass::Util::NormalizedMap.new
@keywords.as_stored.each {|k,v| copied_keywords[k] = v.deep_copy}
copy.__setobj__(new_map)
copy
end

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

Style nit: extra newline.

@nex3

nex3 Jul 26, 2013

Contributor

Style nit: extra newline.

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

removed

@chriseppstein
@@ -1692,6 +1692,35 @@ def test_comment_indentation
SCSS
end
def test_keyword_arguments

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

Also test that with dasherize, underscores become dashes.

@nex3

nex3 Jul 26, 2013

Contributor

Also test that with dasherize, underscores become dashes.

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

added.

@chriseppstein
SASS
$foo: foo($dash-ed: 2px);
SCSS
assert_renders(<<SASS, <<SCSS, :dasherize => false)

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

There's no difference between setting dasherize to false and just not passing it. I don't think this needs both this and the following assertion.

@nex3

nex3 Jul 26, 2013

Contributor

There's no difference between setting dasherize to false and just not passing it. I don't think this needs both this and the following assertion.

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

I like this because it was central to the test and so it made the intent more clear for a reader. But whatever, removed.

@chriseppstein

chriseppstein Jul 30, 2013

Member

I like this because it was central to the test and so it made the intent more clear for a reader. But whatever, removed.

require 'sass/util/normalized_map'
class NormalizedMapTest < Test::Unit::TestCase

This comment has been minimized.

@nex3

nex3 Jul 26, 2013

Contributor

Style nit: extra newline.

@nex3

nex3 Jul 26, 2013

Contributor

Style nit: extra newline.

This comment has been minimized.

@chriseppstein

chriseppstein Jul 30, 2013

Member

removed.

@chriseppstein
### `if()`
The builtin `if()` function allows you to branch on a condition and
evaluates only one of two possible outcomes. It can be used as a value

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

"evaluates" -> "returns", then mention later that unlike a function it only evaluates the argument that gets returned. Also explain why that's useful (e.g. you can safely refer to a variable that may not exist).

I don't like "as a value" here; I'd just say "It can be used in any script context."

@nex3

nex3 Jul 27, 2013

Contributor

"evaluates" -> "returns", then mention later that unlike a function it only evaluates the argument that gets returned. Also explain why that's useful (e.g. you can safely refer to a variable that may not exist).

I don't like "as a value" here; I'd just say "It can be used in any script context."

@@ -194,10 +194,6 @@ module Sass::Script
#
# ## Miscellaneous Functions
#
# \{#if if($condition, $if-true, $if-false)}
# : Returns one of two values, depending on whether or not `$condition` is
# true.

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

I'd keep this here, since this is a place people will look if they see if() in some Sass and want to look it up. Then link to the reference instead of the method doc.

@nex3

nex3 Jul 27, 2013

Contributor

I'd keep this here, since this is a place people will look if they see if() in some Sass and want to look it up. Then link to the reference instead of the method doc.

node(Script::Tree::Funcall.new(tok.value, args, keywords, splat),
tok.source_range.start_pos, source_position)
fn_node = if tok.value == "if"
raise SyntaxError.new("Cannot use ... with if()") if splat

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

I don't think this is a necessary restriction. We can't have special evaluation behavior with a splat, but we don't have to bail on it.

The easiest way to implement this would probably be to keep the old if function, mark it @private, and use that if a splat exists.

@nex3

nex3 Jul 27, 2013

Contributor

I don't think this is a necessary restriction. We can't have special evaluation behavior with a splat, but we don't have to bail on it.

The easiest way to implement this would probably be to keep the old if function, mark it @private, and use that if a splat exists.

module Sass::Script::Tree
# A SassScript parse node representing an inline if function.
class IfFunction < Funcall

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

Style nit: extra newline.

@nex3

nex3 Jul 27, 2013

Contributor

Style nit: extra newline.

# A SassScript parse node representing an inline if function.
class IfFunction < Funcall
# Create the IfFunction node

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

Nit: period

@nex3

nex3 Jul 27, 2013

Contributor

Nit: period

def initialize(args, keywords)
super("if", args, keywords.dup, nil)
if args.size > 3
raise Sass::SyntaxError.new("#{args.size} arguments provided to if() but only 3 are accepted.")

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

This error should look like Ruby argument errors: wrong number of arguments (#{args.size} for 3) forif``

@nex3

nex3 Jul 27, 2013

Contributor

This error should look like Ruby argument errors: wrong number of arguments (#{args.size} for 3) forif``

raise Sass::SyntaxError.new("A condition is required for if()") unless condition
raise Sass::SyntaxError.new("A true value is required for if()") unless true_expression
raise Sass::SyntaxError.new("A false value is required for if()") unless false_expression

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

These should also look like normal argument errors. For example, if() should produce wrong number of arguments (0 for 3) forif``, while if($foo: 12) should produce `Function if requires an argument named $condition`.

@nex3

nex3 Jul 27, 2013

Contributor

These should also look like normal argument errors. For example, if() should produce wrong number of arguments (0 for 3) forif``, while if($foo: 12) should produce `Function if requires an argument named $condition`.

if (unknown_keys = keywords.keys - %w(condition if_true if_false)).any?
raise Sass::SyntaxError.new("$#{unknown_keys.first} is not allowed as an argument to if()")
end
end

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

There should be error-checking for an argument passed both positionally and by name (e.g. if(true, 1, 2, $condition: false)).

@nex3

nex3 Jul 27, 2013

Contributor

There should be error-checking for an argument passed both positionally and by name (e.g. if(true, 1, 2, $condition: false)).

args[1] || keywords["if-true"]
end

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

Style nit: extra newline

@nex3

nex3 Jul 27, 2013

Contributor

Style nit: extra newline

module Sass::Script::Tree
# A SassScript parse node representing an inline if function.
class IfFunction < Funcall

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

I don't think this gets enough value from Funcall to warrant extending it. It seems like it should be its own class, with straightforward ivars for condition, true_expression, and false_expression.

@nex3

nex3 Jul 27, 2013

Contributor

I don't think this gets enough value from Funcall to warrant extending it. It seems like it should be its own class, with straightforward ivars for condition, true_expression, and false_expression.

# @see Node#deep_copy
def deep_copy
IfFunction.new(@args.map{|a| a.deep_copy}, @keywords.deep_copy)

This comment has been minimized.

@nex3

nex3 Jul 27, 2013

Contributor

Style nit: space before {.

@nex3

nex3 Jul 27, 2013

Contributor

Style nit: space before {.

@nex3

This comment has been minimized.

Show comment
Hide comment
@nex3

nex3 Jul 27, 2013

Contributor

This should probably be merged with the NormalizedMap commit.

Contributor

nex3 commented on 698f303 Jul 27, 2013

This should probably be merged with the NormalizedMap commit.

@nex3

This comment has been minimized.

Show comment
Hide comment
@nex3

nex3 Jul 27, 2013

Contributor

Done reviewing.

Contributor

nex3 commented Jul 27, 2013

Done reviewing.

@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Jul 27, 2013

Member

After reading many of your comments, I feel like a better implementation may be to add a generic ability to delay performing certain arguments and pass them to the ruby function as a node that can be performed inside the function's implementation instead of being performed before calling.

Member

chriseppstein commented Jul 27, 2013

After reading many of your comments, I feel like a better implementation may be to add a generic ability to delay performing certain arguments and pass them to the ruby function as a node that can be performed inside the function's implementation instead of being performed before calling.

@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Sep 26, 2013

Member

Closing this in favor of #934.

Member

chriseppstein commented Sep 26, 2013

Closing this in favor of #934.

@nex3 nex3 deleted the if_builtin branch Feb 21, 2014

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