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

Remove support for ={...} substitutions or make it optional #16

Closed
rowanseymour opened this issue Oct 31, 2016 · 8 comments
Closed

Remove support for ={...} substitutions or make it optional #16

rowanseymour opened this issue Oct 31, 2016 · 8 comments
Assignees

Comments

@rowanseymour
Copy link
Member

I'm not sure why this was added because it's neither actual HAML or Django. It breaks existing templates where users have used things like {href: "foo={{ bar }}"}.

I'd like to either remove it for a version 1.0 release, or at least make a way to disable it.

@Psycojoker
Copy link
Collaborator

I'm not fully at ease with breaking backward compatibility but the fact that this is breaking valid django syntax is a problem. I think that it's there to provide an alternative to ruby's #{var}. This is apparently there to remove another older syntax https://github.com/jessemiller/HamlPy/blob/acb79e14381ce46e6d1cb64e7cb154751ae02dfe/hamlpy/elements.py#L135

It would be great to see if this syntax is used somewhere but github search seems broken for that:
https://github.com/search?utf8=%E2%9C%93&q=%22%3D%7B%22+extension%3Ahaml+extension%3Ahamlpy&type=Code&ref=advsearch&l=&l=

@rowanseymour
Copy link
Member Author

Breaking backward compatibility is certainly to be avoided where possible, but I think if we made a 1.0 major release then it could be justified on the basis that:

  • It's not HAML and doesn't provide any advantages over the actual HAML syntax of #{...}
  • It can break templates using Django {{...}} tags, which users like us have been using with the old 0.82 release of hamlpy.

For the sake of users who have used ={...} we could make support configurable - but I do feel that even then the default should be disabled, and the documentation should warn users against using both {{...}} and ={...} in the same templates.

@rowanseymour
Copy link
Member Author

@Psycojoker Github search doesn't support symbol characters apparently. We need to move forward sooner than later on this, and as I see it there are 2 options:

  1. Remove support for ={ (and any other non-HAML oddities?) and make a 1.0 release. Breaking backward compatibility isn't ideal but having a major version release sets appropriate user expectations.
  2. Make support for ={ configurable via a setting

I prefer 1 but am ok with 2 if that's your desire.

@rowanseymour rowanseymour self-assigned this Nov 10, 2016
@rowanseymour
Copy link
Member Author

Had a go at option 2 (see https://github.com/Psycojoker/django-hamlpy/compare/master...Psycojoker:optional_sub_syntax?expand=1) tho still feel removing the syntax would be preferable. Your call. What you think?

@Psycojoker
Copy link
Collaborator

Why would you be against option 2 here? You branch seems pretty good (and, while I prefere simplicity, maybe the default option mecanism + option for rendering could be useful for the futur)

I would prefer not to break backward compatibility but that's not an ultimate no-go for me in this case.

@rowanseymour
Copy link
Member Author

I think we have much better chance of adoption and continued use if we are as close to actual HAML as possible. So this is my proposal:

  1. We make it optional
  2. We remove it from the reference documentation
  3. At some point in future we make a 1.0 release which drops support for it

I can turn the branch above into a PR that does 1 and 2.

@Psycojoker
Copy link
Collaborator

Works for me, just add "display a deprecation warning" between 2 and 3.

@rowanseymour
Copy link
Member Author

Added in #24. Default is that support is continues to be enabled, so no deprecation needed just yet.

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

No branches or pull requests

2 participants