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

Implement custom functions. Resolves #13. #44

Merged
merged 6 commits into from Feb 4, 2015
Merged

Implement custom functions. Resolves #13. #44

merged 6 commits into from Feb 4, 2015

Conversation

asottile
Copy link
Member

@asottile asottile commented Feb 1, 2015

So Yelp only really needs custom functions for strings, but I decided to implement all of the types.

This is my first attempt at writing CPython code so I'm probably doing a bunch of things wrong. Advice would be great here :)

I had a lot of fun writing this and took a lot of inspiration from https://github.com/sass/perl-libsass/blob/master/lib/CSS/Sass.xs

@coveralls
Copy link

Coverage Status

Coverage increased (+3.06%) to 76.04% when pulling 4227f4f on asottile:custom_functions into b043a03 on dahlia:python.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.06%) to 76.04% when pulling 2c41388 on asottile:custom_functions into b043a03 on dahlia:python.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+3.06%) to 76.04% when pulling 2c41388 on asottile:custom_functions into b043a03 on dahlia:python.

blinged_args = ['$' + arg for arg in argspec.args]
signature = '{0}({1})'.format(func_name, ', '.join(blinged_args))
custom_function_list.append((signature.encode('UTF-8'), func))
return custom_function_list
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if there is a type for custom function. The type could have explicit fields about sass function: its name, parameter names, and an arbitrary callable. It also could have a method that returns its signature string. I wonder what’s your thought about this idea.

@dahlia
Copy link
Member

dahlia commented Feb 2, 2015

I don’t think data types have to be defined in a separated module. They could be placed in sass.py together.

@dahlia
Copy link
Member

dahlia commented Feb 2, 2015

return super(SassWarning, cls).__new__(cls, msg)


class SassMap(dict):
Copy link
Member

Choose a reason for hiding this comment

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

Is it good idea to subclass dict to define a immutable mapping type? How about subclassing collections.Mapping?

@dahlia
Copy link
Member

dahlia commented Feb 2, 2015

Anyway I really appreciate you! It would be the killer feature of the next release.

@asottile
Copy link
Member Author

asottile commented Feb 2, 2015

I'll respond all at once instead of a bunch of comments so it's easier:

  • custom function map
    So I actually went back and forth a bunch on this :/ My initial implementation mapped signature to function, but that duplicates all of the information that we can just derive from the function itself (and then we have to do a bunch of validation). This also seems to have a nice balance between ease of use to callers and correctness.
  • sassutils.sass_types: I initially put them in sassutils because I was worried about circular imports. However that's not really a problem in c-api, I'll move them to the sass module instead!
  • Hmm, I'll try and fix windows when I get home.
  • I can change it to Mapping pretty easily, I'll do that.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.48%) to 75.47% when pulling 0a9e498 on asottile:custom_functions into b043a03 on dahlia:python.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling 8a6f37d on asottile:custom_functions into b043a03 on dahlia:python.

@asottile
Copy link
Member Author

asottile commented Feb 3, 2015

I'm having a really hard time debugging the windows errors :( It would seem it only happens in release mode as well so I lose most of the debugger symbols.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling 2dda11f on asottile:custom_functions into b043a03 on dahlia:python.

def _immutable(self, *_):
raise TypeError('SassMaps are immutable.')

__setitem__ = __delitem__ = _immutable
Copy link
Member

Choose a reason for hiding this comment

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

I think these _immutable/__setitem__/__delitem__ methods don’t have to be overridden anymore, because they actually aren’t defined by collections.Mapping! It is immutable by default, and there’s collections.MutableMapping instead for additional mutability.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I thought these were better error messages, I can remove however

@dahlia
Copy link
Member

dahlia commented Feb 3, 2015

My initial implementation mapped signature to function, but that duplicates all of the information that we can just derive from the function itself (and then we have to do a bunch of validation). This also seems to have a nice balance between ease of use to callers and correctness.

I am almost with you. For ease of use I think we can make compile() function possible to take multiple types e.g.:

def func_name():
    pass

# The most general form: passing a set of SassFunctions.
# It can take any kind of callables, but pretty verbose.
compile(
    custom_functions={  # I just suggest this more general parameter name instead
        SassFunction('func_name', (), func_name)
    }
)

# Less general, but easier-to-use form: passing a mapping.
# Although it cannot take any kind of callables, it can take any kind of *functions*
#   defined using `def`/`lambda` syntax.
# It cannot take callables other than them since inspecting arguments is not
#   always available for every kind of callables.
compile(
    custom_functions={
        'func_name': lambda: ...
    }
)

# Not general, but the easiest-to-use form for *named* functions:
#   passing a set of named functions.
# It can take only named functions which mean defined using `def`.
# It cannot take lambdas since names are unavailable for them.
compile(
    custom_functions={func_name}
)

@asottile
Copy link
Member Author

asottile commented Feb 3, 2015

That's a lot of complexity imo. I can take a stab at it however

@dahlia
Copy link
Member

dahlia commented Feb 3, 2015

Okay, so let’s keep the current implementation. How about the parameter name I suggested?

@asottile
Copy link
Member Author

asottile commented Feb 3, 2015

Name works for me, I'll change it :)

@asottile
Copy link
Member Author

asottile commented Feb 3, 2015

So I've determined the windows issue is actually a problem with upstream. Would it be acceptable to simply disable the error-handling tests under windows?

@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling e3c3f57 on asottile:custom_functions into b043a03 on dahlia:python.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling e3c3f57 on asottile:custom_functions into b043a03 on dahlia:python.

@dahlia
Copy link
Member

dahlia commented Feb 4, 2015

Would it be acceptable to simply disable the error-handling tests under windows?

That would be fine if it’s marked as FIXME.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling efa2b9f on asottile:custom_functions into b043a03 on dahlia:python.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling efa2b9f on asottile:custom_functions into b043a03 on dahlia:python.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling 82655fa on asottile:custom_functions into b043a03 on dahlia:python.

@asottile
Copy link
Member Author

asottile commented Feb 4, 2015

\o/ I got windows to pass \o/

one stupid compile flag was missing >.<

@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling b5faf60 on asottile:custom_functions into b043a03 on dahlia:python.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+3.31%) to 76.3% when pulling b5faf60 on asottile:custom_functions into b043a03 on dahlia:python.

@dahlia
Copy link
Member

dahlia commented Feb 4, 2015

Thanks for you again. 😄

dahlia added a commit that referenced this pull request Feb 4, 2015
Implement custom functions.  Resolves #13.
@dahlia dahlia merged commit 1353f25 into sass:python Feb 4, 2015
@dahlia
Copy link
Member

dahlia commented Feb 4, 2015

I am going to release the next version (probably 0.7.0) which includes this.

@asottile asottile deleted the custom_functions branch February 4, 2015 16:10
@asottile
Copy link
Member Author

asottile commented Feb 4, 2015

Thanks!

dahlia added a commit that referenced this pull request Mar 4, 2015
dahlia added a commit that referenced this pull request Mar 5, 2015
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.

None yet

3 participants