Talesoft / tale-jade

A complete and fully-functional implementation of the Jade template language for PHP
http://jade.talesoft.codes
MIT License
88 stars 10 forks source link

Allow arbitrary spacing/indentation in php code block and text block (multiline) #44

Closed elquimista closed 8 years ago

elquimista commented 8 years ago

This is kind of annoying bug to my experience. Let's take an example:

-
    /**
     * CakePHP(tm) : Rapid Development Framework (http://cakephp.org)
     * Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
     */

doctype html

This snippet gives me Jade parse error, it says like this: Failed to parse jade: Mixed indentation style encountered. Dont mix tabs and spaces. Stick to one of both. (Line: 3, Offset: 2)

TorbenKoehn commented 8 years ago

True. The problem here is that the * in every following line is indented one space more than the previous lines.

I'll look if I can find an elegant fix for that.

TorbenKoehn commented 8 years ago

http://sandbox.jade.talesoft.io/id-56993b264554f.html

Honestly, I checked my own code, this exception really only gets thrown if you mix tabs and spaces somewhere.

Could you check that again and give me feedback if it still happens?

TorbenKoehn commented 8 years ago

Forgot to mention it, I implemented some kind of conversion mechanism in 77e539532b402f7c2cf970978e29e5748b60c4d6.

It will try to convert tabs to spaces, when encountered, if it already has recognized that it is using spaces with a fixed width.

To be able to have converted tabs from the begin on, you should pass some lexer options

'lexerOptions' => [
    'indentStyle' => ' ',
    'indentWidth' => 2 //or 4, whatever you indent with
]

Please tell me if it's fixed for you :)

elquimista commented 8 years ago

I tried with the following configuration, but it's still not working;

$options = [
    'adapterOptions' => [
        'path' => 'tmp/cache/views'
    ],
    'lexerOptions' => [
        'indentStyle' => ' ',
        'indentWidth' => 4
    ],
    'pretty' => true,
];

$renderer = new Jade\Renderer($options);
TorbenKoehn commented 8 years ago

It has to be a tab somewhere in the code.

Can you upload the exact files you used somewhere in their original state?

I'll take a second look at it

elquimista commented 8 years ago

Here's the file 1.jade 1.jade.txt

TorbenKoehn commented 8 years ago

Well you could remove the tabs, but the lexer options there should also be able to fix that, I'll take a look at it

TorbenKoehn commented 8 years ago

The test I added takes your exact file you sent me (including tabs) and sets the lexer options to spaces.

It renders correctly and the tests pass.

Do you have any other hints I could use to find the problem?

elquimista commented 8 years ago

It still doesn't work for me. Can you test it with actual file rather than using string variable as contents?

TorbenKoehn commented 8 years ago

I'd merge the fix right now, but there seems to be a problem with Travis updating composer.

I can't get the build to pass (It passes on my local machine, 5.6)

I'll update this when it works again (or I resolved this in any other way)

TorbenKoehn commented 8 years ago

Can you update and test it again?

elquimista commented 8 years ago

Just tested. It works only with indentWidth => 2, otherwise it behaves weirdly.

elquimista commented 8 years ago

I guess default indentWidth is 2, but it throws an error without explicitly specifying indentWidth in lexerOptions. Anyway, mission complete :+1:

TorbenKoehn commented 8 years ago

Ill still optimize this, working on it, but I'll close this for now if everything is okay now.

I'll update it later when I changed it.

elquimista commented 8 years ago

Wait. I have a problem. Now the lexer seems more strict than before. And I'm getting following exception message: Failed to lex jade: You should indent in by one level only (Line: 2, Offset: 4) There's no template file path indicated.

elquimista commented 8 years ago

I'm writing an urgent code based on Tale Jade template engine. Can you revert the commit for this issue? Or do you have any idea for an elegant solution?

TorbenKoehn commented 8 years ago

Please give me the code you're using, as a text file in the best case, like the last time.

The best way to avoid these things is just using spaces for indentation. There seem to be some issues when tabs are involved that I am fixing right now.

elquimista commented 8 years ago

for_members.ctp.jade.txt

This one uses spaces as indentation. And it throws an exception: Failed to compile Jade: Failed to lex jade: You should indent in by one level only (Line: 4, Offset: 4) [/var/www/html/src/Template/Layout/for_members.ctp.jade]

elquimista commented 8 years ago

for_members.ctp.jade.txt

And this one uses tabs as indentation. Thrown exception in this case: Failed to lex jade: You should indent in by one level only (Line: 2, Offset: 4) And no template file path is displayed.

elquimista commented 8 years ago

FYI, I'm using indentWidth = 2 for lexerOptions

TorbenKoehn commented 8 years ago

I've just re-released 1.3.6, could you update and test again?

elquimista commented 8 years ago

Updated and tested. Now it throws a different exception: Failed to parse Jade: The expression instruction can't have children (indent at 24:4) Still not specifying origin template file. Can you please fix it so that we can get back the file path?

elquimista commented 8 years ago

Sandbox uses the latest Tale Jade? I guess it doesn't. Just tried the following code snippet and it works fine on sandbox:

doctype html
<html>
<head>
    != $view->Html->charset()
    <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1">

, which is not working in my script on the remote server and throwing me an exception as follows: Failed to parse Jade: The expression instruction can't have children (indent at 24:4) Again, there's no template file path. I just figured it out the origin file on my own.

TorbenKoehn commented 8 years ago

On it

TorbenKoehn commented 8 years ago

Can you test it without any lexer options?

TorbenKoehn commented 8 years ago

I added a bunch of tests taking the files you gave me as well as the snippet you just posted, all tests path, I don't get the exception.

What else in your files could it be? It's pretty weird that the file name isn't printed, since everything that gets parsed through the compiler should have a file-name, or did you call ->compile() somewhere directly?

elquimista commented 8 years ago

Sorry, looks like my browser's problem. But I just tested and this time nesting elements don't render properly.

body
    header
        nav.white(role="navigation")
            .nav-wrapper.container

doesn't render properly in terms of nesting. It renders as follows:

<body>
  <header></header>
  <nav class="white" role="navigation">
    <div class="nav-wrapper container"></div>
  </nav>
</body>
TorbenKoehn commented 8 years ago

http://sandbox.jade.talesoft.io/id-569aada5391eb.html

It renders correctly here using the latest version

elquimista commented 8 years ago

ok. updated to the latest and it works like a charm. PERFECT!! thanks for your great work

elquimista commented 8 years ago

btw, I had to remove lexerOptions in order to make sure everything work properly. so I think we no longer need that option.

TorbenKoehn commented 8 years ago

The option has a different meaning, it actually lets you define the initial indentation style.

This is useful if your first line already starts with indentation for whatever reason and you want to let it know what style it has.