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

Pyjade doesn't support element substitution #238

Open
smaudet opened this issue Dec 29, 2015 · 4 comments
Open

Pyjade doesn't support element substitution #238

smaudet opened this issue Dec 29, 2015 · 4 comments

Comments

@smaudet
Copy link

smaudet commented Dec 29, 2015

It appears this is because of lack of support for mixins in mixins. The following should work:

mixin something(element,id)
  #{element}(href='blah',id='junk#{id}').doggy Hello World

mixin something2(id)
  +something('div',id)

+something('p',1)
+something2(2)

Should produce

<p href="blah" id="junk1" class="doggy">Hello World</p>
<div href="blah" id="junk2" class="doggy">Hello World</div>

However this gives some bogus text in pyjade.

You've not implemented the interpreter correctly in pyjade. I suspect you have a large, large number of problems in pyjade, to the extent that this whole project is maybe throwaway, without rewriting the interpreter.

Additionally, tracked down the bug to the lexer - pyjade sees the

 #{element}(href='blah',id='junk#{id}').doggy Hello World

barfs, and assumes it is text. I presume the fix involves teaching the lexer that this is in fact in need of interpolation. Not only is this significantly broken, this is also looking like a significant amount of work to fix.

Looking at the Lexer, it seems all that it does is look for REGEX patterns to determine what the current element is. It may be that all we need to do is copy the interpolation from the pug-js jade-lexer package (https://github.com/pugjs/pug-lexer/blob/master/index.js), and then we may be golden...however I don't know how much support this would involve to port the feature everywhere else.

I'm thinking it may just be a lot simpler to use pyjs to pipe to/from python directly, rather than re-inventing the wheel here...

@char101
Copy link
Contributor

char101 commented Dec 30, 2015

You've not implemented the interpreter correctly in pyjade. I suspect you have a large, large number of problems in pyjade, to the extent that this whole project is maybe throwaway, without rewriting the interpreter.

I think your comment is too harsh. I implemented the fix for this without much problem.

#239

mixin test(element, value)                                                                                                                                                                                                                                                                                                  
  #{element} value = #{value}                                                                                                                                                                                                                                                                                               

mixin test2(value)                                                                                                                                                                                                                                                                                                          
  +test('test2', value)                                                                                                                                                                                                                                                                                                     

mixin test3(element3)                                                                                                                                                                                                                                                                                                       
  #{element3} content                                                                                                                                                                                                                                                                                                       

mixin test4(text4)                                                                                                                                                                                                                                                                                                          
  p #{text4}                                                                                                                                                                                                                                                                                                                

+test('div', 'hello')                                                                                                                                                                                                                                                                                                       

+test2('world')                                                                                                                                                                                                                                                                                                             

+test3('div')                                                                                                                                                                                                                                                                                                               

+test4('text 4')
/temp/pyjade/element-interpolation # ./env/bin/pyjade test.jade
WARNING:root:No module named 'django'
WARNING:root:No module named 'tornado'
<div>value = hello</div>
<test2>value = world</test2>
<div>content</div>
<p>text 4</p>

@smaudet
Copy link
Author

smaudet commented Dec 30, 2015

Perhaps. I took a look at your changes, and they seemed to be the ones I mainly suspected would be necessary.

I'll take a look over changes and merge them with mine...where I suspect your merge commit may still fall short though is support for all the various extensions. You might have tested html.py, but then there's a lot of work I'd imagine for django/jinja etc. (not to mention, tests to write).

Additionally, while your commit is relatively small, and I don't know how familiar you already were with this codebase, for a newcomer this seems like a hassle to port new features from jade to the compiler tree. Most of the work of fixing this bug (so far) was finding the failing portion in pyjade, understading pyjade's innards, and then correlating that with the jade compiler. You potentially had my help with finding the correct places to make a change, however without that I don't think this change would be so easy to make.

What do you propose, keeping an eye on any/all commits/PRs that come into the jade project(s) and then immediately porting them to pyjade? Seems like a lot of (wasted) work...

But thanks for looking at this! :)

@char101
Copy link
Contributor

char101 commented Dec 31, 2015

I don't use node and I have no assumption that this project is 100% compatible with jade. My use case is I want a jade like templating language and I only use it with jinja. Thus my pull request will only touch the core and the jinja extension. I'll let other people that use django/mako to update the other extensions since I don't have much experience with those templating languages.

I implemented #239 mainly from looking at the jade commit. The trick is to add the buffer attribute to the Tag node. The rest is easy, finding when to set the buffer attribute (modifying the lexer and then settting the buffer attribute based on the matched regex) and modifying the compiler output when the buffer attribute is set.

@smaudet
Copy link
Author

smaudet commented Dec 31, 2015

I use jinja as well, my goal is slightly different in that I like jade-lang, but I want to use flask (which happens to use jinja - I'm not really familiar with any of these templating languages other than jade).

I have no assumption that this project is 100% compatible with jade

That's fair, it would be nice if, as we find/fix problems in pyjade, we document which features are supported by which sub-templating language. It should be relatively easy to do, by simply running the tests (last time i did I got one failure in django and one in jinja).

Additionally, it would be good to make sure there are tests for all new features.

It would be especially good for one of us to document this process of adding/modifying features...somewhere. Maybe not the main README.md, maybe inside the pyjade directory? I'm pretty sure I've run into at least 3 or 4 features pyjade lacks which exist within the language proper. I've not gotten around to reporting all of them, though, and not all of them are as critical as this one was.

Still, if we can create a backlog/easy way of updating for new features...

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

No branches or pull requests

2 participants