Skip to content

Upgrade libsass to 3.5.0.beta1 #184

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

Merged
merged 1 commit into from
Jan 17, 2017
Merged

Upgrade libsass to 3.5.0.beta1 #184

merged 1 commit into from
Jan 17, 2017

Conversation

asottile
Copy link
Member

Yay new beta!

Upstream has an abi breaking change which translates to a breaking change for us (SassList now has an additional required parameter)


def __new__(cls, items, separator):
def __new__(cls, items, separator, bracketed):
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 the added parameter has a default value for backward compatibility. 🤔

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 considered that as well but wasn't really sure what to put. Both values seem equally wrong to me (though False was the only possible value before today).

On the other hand, it's super unlikely that consumers are even using this feature. At least internally we're only using sass functions to return strings and don't come close to dealing with maps or lists.

I guess I could change this to bracketed=False?

Copy link
Member

Choose a reason for hiding this comment

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

If you meant giving any default value (True or False) is inappropriate, how about making this parameter to be False by default, but warn (i.e. DeprecationWarning) when it's not explicit?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just set it False by default. After a good night's sleep I figured this is a fine default since that used to be the only way.

@coveralls
Copy link

coveralls commented Jan 12, 2017

Coverage Status

Coverage increased (+0.03%) to 94.488% when pulling 919e415 on 3_5_beta1 into 5bcefdd on master.

@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

Coverage increased (+0.03%) to 94.488% when pulling d0d8ece on 3_5_beta1 into 5bcefdd on master.

Copy link
Member

@dahlia dahlia left a comment

Choose a reason for hiding this comment

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

👍

@dahlia dahlia merged commit f8cd532 into master Jan 17, 2017
@asottile asottile deleted the 3_5_beta1 branch January 17, 2017 14:45
asottile added a commit that referenced this pull request Jun 7, 2017
This reverts commit f8cd532, reversing
changes made to 5bcefdd.
asottile added a commit that referenced this pull request Feb 10, 2018
asottile added a commit that referenced this pull request Mar 6, 2018
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.

3 participants