Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Operating System: other
      Platform: Other

      Description

      I'd like to allow all directives be able to stretch across multiple lines. An
      example might be:

      #set ($a =
      10)

      or

      #if( $biglongreference ==
      "very big long string")

      The rationale for this is that newlines are sometimes silently inserted by web
      design applications into arbitrary HTML. For example, many of our users work
      with Macromedia DreamWeaver. DreamWeaver is not aware of VTL, and treats it
      like ordinary text. A user might type VTL in the "HTML" mode of DreamWeaver,
      only to see the VTL reformatted as paragraphed text after
      editing/saving/reloading. This means that a web page with VTL might work fine
      one day, and then break after the page is loaded/saved by the tool. This can
      be highly confusing, especially to the less technical template writers who
      might be using the web design program in the first place.

      In addition (although this is a less important reason), most scripting
      languages permit newlines within control statements.

      A couple of notes about this bug report and patch.

      (1) It's 100% backwards compatible, and passes "ant test"
      (2) The change is simple (despite the long patch). I simply replaced most
      references to <WHITESPACE> in Parser.jjt with <WHITESPACE_NEWLINE>
      (3) This is a completely separate issue from any debates over gobbling
      whitespace (Bug# 7940) or allowing multiline literals (Bug # 17803).
      (4) Whitespace gobbling rules at start/end of directive remain the same. (I
      think this should be addressed separately).

      1. ASF.LICENSE.NOT.GRANTED--multilinedirective.vm
        0.1 kB
        Will Glass-Husain
      2. ASF.LICENSE.NOT.GRANTED--multilinedirective.cmp
        0.0 kB
        Will Glass-Husain
      3. ASF.LICENSE.NOT.GRANTED--test.patch
        0.5 kB
        Will Glass-Husain
      4. ASF.LICENSE.NOT.GRANTED--Parser.jjt.patch
        4 kB
        Will Glass-Husain

        Activity

        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=8875)
        patch to Parser.jjt

        Show
        Will Glass-Husain added a comment - Created an attachment (id=8875) patch to Parser.jjt
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=8876)
        patch to template.properties

        Show
        Will Glass-Husain added a comment - Created an attachment (id=8876) patch to template.properties
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=8877)
        new file: multilinedirective.cmp

        Show
        Will Glass-Husain added a comment - Created an attachment (id=8877) new file: multilinedirective.cmp
        Hide
        Will Glass-Husain added a comment -

        Created an attachment (id=8878)
        new file: multilinedirective.vm

        Show
        Will Glass-Husain added a comment - Created an attachment (id=8878) new file: multilinedirective.vm
        Hide
        Geir Magnusson Jr added a comment -

        IIRC, I had an example that people were playing with a while ago... I think my impl was simpler.
        Let me go see...

        Show
        Geir Magnusson Jr added a comment - IIRC, I had an example that people were playing with a while ago... I think my impl was simpler. Let me go see...
        Hide
        Will Glass-Husain added a comment -

        Great! This patch would be helpful to our users.

        By the way, this is really only a one-line patch. Changing the <WHITESPACE>
        token to include \n and \r is sufficient. But I thought it useful (for self-
        documentation purposes) to change the token name to <WHITESPACE_NEWLINE>.

        Show
        Will Glass-Husain added a comment - Great! This patch would be helpful to our users. By the way, this is really only a one-line patch. Changing the <WHITESPACE> token to include \n and \r is sufficient. But I thought it useful (for self- documentation purposes) to change the token name to <WHITESPACE_NEWLINE>.
        Hide
        Nathan Bubna added a comment -

        on the semantics of "whitespace" vs. "whitespace_newline", i don't like the
        latter one. it seems to imply either that newline characters are not
        whitespace, but IMO, they definitely are and should be treated as such. as
        such, i'm eager for this patch (and all others that treat newline chars as mere
        whitespace), but don't like the semantic change.

        Show
        Nathan Bubna added a comment - on the semantics of "whitespace" vs. "whitespace_newline", i don't like the latter one. it seems to imply either that newline characters are not whitespace, but IMO, they definitely are and should be treated as such. as such, i'm eager for this patch (and all others that treat newline chars as mere whitespace), but don't like the semantic change.
        Hide
        Daniel Rall added a comment -

        I'd like to echo Nathan's preference for the semantics of whitespace – newlines
        should be treated as whitespace.

        Show
        Daniel Rall added a comment - I'd like to echo Nathan's preference for the semantics of whitespace – newlines should be treated as whitespace.
        Hide
        Geir Magnusson Jr added a comment -

        done in simplest possible way.

        Show
        Geir Magnusson Jr added a comment - done in simplest possible way.

          People

          • Assignee:
            Unassigned
            Reporter:
            Will Glass-Husain
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development