Skip to content

fix parser error on '…\n…' template expression #80

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

Closed
wants to merge 1 commit into from

Conversation

kamikat
Copy link

@kamikat kamikat commented Dec 20, 2018

Change to accept template strings like:

pug`\n  div\n    p test\n`

Which is identical to

pug`
  div
    p test
`

@codecov-io
Copy link

codecov-io commented Dec 20, 2018

Codecov Report

Merging #80 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #80   +/-   ##
=======================================
  Coverage   82.24%   82.24%           
=======================================
  Files          20       20           
  Lines         445      445           
  Branches      103      103           
=======================================
  Hits          366      366           
  Misses         70       70           
  Partials        9        9
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 429ca31...a607b8e. Read the comment docs.

@kamikat kamikat force-pushed the patch-1 branch 2 times, most recently from a02abad to 71776db Compare December 21, 2018 03:38
@kamikat kamikat force-pushed the patch-1 branch 5 times, most recently from c9536ed to a607b8e Compare December 21, 2018 09:06
@ezhlobo
Copy link
Member

ezhlobo commented Dec 27, 2018

@kamikat hello! Could you please give more context why is it useful? Is there a reason to write new lines by hands instead of using indentation as it's how pugjs works?

@kamikat
Copy link
Author

kamikat commented Jan 4, 2019

Given following tagged template string in CoffeeScript:

pug"""
  div
    p test
"""

which compiles to:

pug`div\n  p test`;

@ezhlobo
Copy link
Member

ezhlobo commented Feb 10, 2019

@kamikat hey, something inside me really afraid of having this in the codebase. I don't have rational reason against it though.

While I'm so cautious and in the middle of thinking process, are you able to fork and use your fork as a dependency in your project? I don't want my delay be a blocker for you.

@kamikat
Copy link
Author

kamikat commented Feb 11, 2019

I've been used my own fork for a while.

And yes, this is a quick hack to solve the problem with coffee compiler without enough robustness.

I'll open another feature request for this.

@kamikat kamikat closed this Feb 11, 2019
@ForbesLindesay
Copy link
Member

These two strings are not identical. This is intentional:

pug`
  pre= 'Hello\nWorld'
`;

Is not the same as:

pug`
  pre= 'Hello
World'
`;

The first is valid pug code, the later is not. I realise this sucks for CoffeeScript users, but I don't think that should be a priority for us here.

@audunolsen
Copy link

I believe having whitespace sensitive syntax throughout the whole codebase, at least coherently throughout a single react cjsx file has great appeal. Hoping this isn't dismissed entirely.

@audunolsen
Copy link

I've done some quick research and discovered that the there's active work in the CoffeeScript repo to separate explicit and implicit newlines in template literals it outputs. The discussion spawned from someone having an issue pretty identical to this one, only a different template literal tag.

A workaround as of today is to use the CoffeeScript "ast" branch, I've tested it myself and pug tagged strings compile flawlessly. Put the following in your package.json:
"coffeescript": "jashkenas/coffeescript#ast"

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

Successfully merging this pull request may close these issues.

5 participants