Skip to content
This repository was archived by the owner on Aug 17, 2023. It is now read-only.

Make ={...} syntax support optional #24

Merged
merged 9 commits into from
Dec 2, 2016
Merged

Conversation

rowanseymour
Copy link
Member

@rowanseymour rowanseymour commented Nov 16, 2016

This PR punts on the question of deprecating ={...} for now, and just makes it optional using the HAMLPY_DJANGO_INLINE_STYLE setting.

I still think deprecation is the way to go, especially once we add support for .(attr=val) syntax which introduces yet another meaning for =. I just want more time to think through the path to becoming more HAML and less HAMLPY. It bothers me that we'll be supporting three different syntaxes for attributes (.{attr:val}, .{attr=>val} and .(attr=val)) but dropping support for syntax that people are using is always tricky.

Addresses #16. @Psycojoker please review

@rowanseymour rowanseymour self-assigned this Nov 16, 2016
@coveralls
Copy link

coveralls commented Nov 16, 2016

Coverage Status

Coverage decreased (-0.07%) to 80.413% when pulling bb5aaf4 on optional_sub_syntax into bf64dde on master.

@Psycojoker
Copy link
Collaborator

It would be great to find a way to throw a depreciation warning if we find that the old syntax is still used instead of disabling it by default and breaking on a "pip -U".

I don't know if it's a lot of work :/

@rowanseymour
Copy link
Member Author

@Psycojoker agreed but this PR doesn't disable it by default - this PR is completely backwards compatible

@coveralls
Copy link

coveralls commented Nov 23, 2016

Coverage Status

Coverage decreased (-0.05%) to 79.122% when pulling a3a20fa on optional_sub_syntax into 038d993 on master.

@Psycojoker
Copy link
Collaborator

Oh, my bad, shouldn't review tired after work, sorry :/

@rowanseymour
Copy link
Member Author

Np. Did you actually get a chance to look at the code? Would really love to get this merged as we need this to use the latest django-hamlpy with our main project.

@rowanseymour
Copy link
Member Author

@Psycojoker bump. I don't like to pester but this PR has been open almost two weeks and we need it

@coveralls
Copy link

coveralls commented Nov 29, 2016

Coverage Status

Coverage decreased (-0.06%) to 79.333% when pulling da81981 on optional_sub_syntax into 05be260 on master.

@coveralls
Copy link

coveralls commented Dec 1, 2016

Coverage Status

Coverage decreased (-0.005%) to 92.031% when pulling 22dfa7d on optional_sub_syntax into cc7695b on master.

@rowanseymour rowanseymour merged commit dc9a526 into master Dec 2, 2016
@rowanseymour rowanseymour deleted the optional_sub_syntax branch December 2, 2016 14:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants