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

Incorrect splitting of attribute value when line is too long #61

Closed
seletskiy opened this issue Jan 14, 2020 · 10 comments · Fixed by #62
Closed

Incorrect splitting of attribute value when line is too long #61

seletskiy opened this issue Jan 14, 2020 · 10 comments · Fixed by #62
Assignees
Labels
framework: Angular Related to the framework Angular framework: Vue Related to the framework Vue type: bug Functionality that does not work as intended/expected

Comments

@seletskiy
Copy link
Contributor

Before formatting:

div
  table
    tbody
      tr
        td
          a(:href=`Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()`)

After formatting:

div
  table
    tbody
      tr
        td
          a(
            :href='Nav.currentProject()
  .repo(getRepository(pipeline).slug)
  .buildNoContext()'
          )

This is incorrect syntax: no newlines accepted in single quotes.

@Shinigami92 Shinigami92 self-assigned this Jan 14, 2020
@Shinigami92 Shinigami92 added the type: bug Functionality that does not work as intended/expected label Jan 14, 2020
@Shinigami92
Copy link
Member

Shinigami92 commented Jan 14, 2020

Can you workaround it (for quick-resolving) by moving it into a shorter function that will be called?

What would be your expected output?

Also are you sure that :href=`... become :href='...? 🤔

@seletskiy
Copy link
Contributor Author

seletskiy commented Jan 14, 2020 via email

@seletskiy
Copy link
Contributor Author

Also are you sure that :href=`... become :href='...? thinking

Yep, that's also a thing.

@Shinigami92
Copy link
Member

I made some tests on https://pughtml.com

Input

- const getRepository = () => ({ slug: 'slug' })
- const Nav = { currentProject: () => ({ repo: () => ({ buildNoContext: () => 'no-context' }) }) }
- const hasError = true

div
  table
    tbody
      tr
        td
          a(x=1, :href=`Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()`)
        td
          a(x=2, :href='Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()')
        td
          a(x=3, :href=Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext())
        td
          a(x=4, :href=`Nav.currentProject()
          .repo(getRepository(pipeline).slug)
          .buildNoContext()`)
        //- td
        //-    // Syntax Error: Unterminated string constant - line 21:25
        //-    a(x=5, :href='Nav.currentProject()
        //-    .repo(getRepository(pipeline).slug)
        //-    .buildNoContext()')
        td
          a(x=6, :href=Nav.currentProject()
          .repo(getRepository(pipeline).slug)
          .buildNoContext())
      tr
        td
          span(x=7, :class='{ "text-danger": hasError }')
        td
          span(x=8, :class=`{ "text-danger": hasError }`)
        td
          span(x=9, :class={ "text-danger": hasError })
        td
          // Syntactically incorrect HTML
          span(x=10, class='{ "text-danger": hasError }')
        td
          // Syntactically incorrect HTML
          span(x=11, class=`{ "text-danger": hasError }`)
        td
          span(x=12, class={ "text-danger": hasError })

Output

<div>
    <table>
        <tbody>
            <tr>
                <td><a x="1" :href="Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()"></a></td>
                <td><a x="2" :href="Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()"></a></td>
                <td><a x="3" :href="no-context"></a></td>
                <td><a x="4" :href="Nav.currentProject()
          .repo(getRepository(pipeline).slug)
          .buildNoContext()"></a></td>
                <td><a x="6" :href="no-context"></a></td>
            </tr>
            <tr>
                <td><span x="7" :class="{ &quot;text-danger&quot;: hasError }"></span></td>
                <td><span x="8" :class="{ &quot;text-danger&quot;: hasError }"></span></td>
                <td><span x="9" :class="{&quot;text-danger&quot;:true}"></span></td>
                <td>
                    <!-- Syntactically incorrect HTML--><span class="{ &quot;text-danger&quot;: hasError }" x="10"></span></td>
                <td>
                    <!-- Syntactically incorrect HTML--><span class="{ &quot;text-danger&quot;: hasError }" x="11"></span></td>
                <td><span class="text-danger" x="12"></span></td>
            </tr>
        </tbody>
    </table>
</div>

To sum up

  • Test 5 isn't working at all
  • Test 10 and 11 are invalid HTML
  • When using no quotes, the result is calculated immediately (and not managed by vue?)
  • When using a framework like vue or angular, we should prefer data-binding of this framework (I could add this to README.md 🤔)
  • for me: my plugin errors with SyntaxError: Unexpected token (1:71) when I try to format the input 🤔 this error is thrown by Test 3

For your case: What are you expecting? Test-Case 2 or 4?
I would prefer 2, because I think it's easier to fix ^^
If 4, what should be the indentation for wrapped lines? (Please add an expected output in your comment)

@seletskiy
Copy link
Contributor Author

@Shinigami92:

Across the project there are only to variants (and they are correctly formatted by plugin):

  • single quoted value (not wrapped)
  • backtick quoted value where value is a dict, e.g.:
    v-bem:+=`{
                selected:   selected && selected.id === job.id,
                selectable: !!data,
            }`,
    
    I don't like the selected style of formatting (but that's not exactly part of this issue). I would prefer something like that:
    v-bem:+=`{
      selected:   selected && selected.id === job.id,
      selectable: !!data,
    }`,
    

So, to recap: I expect formatting as described in Test Case 2.

BTW Test 3 looks weird. Never seen anything like that before.

@Shinigami92
Copy link
Member

Could you post me your prettier configuration?

The backtick quoted dict value formatting is definitely something that I could address in the near future in it's own issue

@seletskiy
Copy link
Contributor Author

@Shinigami92

prettier.config.js:

module.exports = {
  singleQuote: true,
  trailingComma: 'es5',
  vueIndentScriptAndStyle: true,
};

@Shinigami92 Shinigami92 added framework: Angular Related to the framework Angular framework: Vue Related to the framework Vue labels Jan 15, 2020
@Shinigami92
Copy link
Member

Ok, I workaround an internal use of the prettier library itself
I don't currently know if it's a "wrongly" used function of internal prettier parser or if it's also a bug there...

Nevermind, I will release a 1.1.4 for now so that you can work again

There is another (known) "problem" that a tag with only one long argument is wrapped in it's own line

// so
a(:href='Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()')
// will currently become
a(
  :href='Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()'
)

That's because it's more than 80/printWidth characters long
But as said, this is/could be another issue

@Shinigami92
Copy link
Member

Released v1.1.4
Let me know that everything is working correctly

@seletskiy
Copy link
Contributor Author

Let me know that everything is working correctly

Just tested it. Works fine. Thank you for such a quick fix!

There is another (known) "problem" that a tag with only one long argument is wrapped in it's own line

Well, I personally don't classify it as a problem. Writing such long lines is a bad practice anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework: Angular Related to the framework Angular framework: Vue Related to the framework Vue type: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants