tonyganch / gonzales-pe

CSS parser with support of preprocessors
MIT License
330 stars 66 forks source link

[CSS/LESS/SCSS] Should raise an error for missing semicolons #145

Closed haoqunjiang closed 8 years ago

haoqunjiang commented 8 years ago

From this issue: https://github.com/csscomb/csscomb.js/issues/447

The following css block should not pass the parser:

.test {
    position: absolute
    display: block
    padding: 0 10px 0 0
    margin: auto
    color: red
    border-color: green
}
DanPurdy commented 8 years ago

I'm not sure this is a bug, Isn't this kind of enforcement the job of a linter? A parser should just parse whatever is fed into it whether it's valid or not? For example it's perfectly valid (but bad practice) to leave a semicolon off of the last property in a block, do we really need the parser to be checking all that rather than just giving us a representation of the current state of the code and allowing us to handle it as we like?

tonyganch commented 8 years ago

Hi @DanPurdy! You look amazing today :)

I checked the code above with CSSOM (which is implementation of a standard) <1>. Seems like it parses the example exactly the same way as GPE does, with everything being one big weird property.

<1>: http://nv.github.io/CSSOM/docs/parse.html#css=.test%20%7B%0A%20%20%20%20position%3A%20absolute%0A%20%20%20%20display%3A%20block%0A%20%20%20%20padding%3A%200%2010px%200%200%0A%20%20%20%20margin%3A%20auto%0A%20%20%20%20color%3A%20red%0A%20%20%20%20border-color%3A%20green%0A%7D
DanPurdy commented 8 years ago

Hi @tonyganch i try my best :wink: how're you?

That's what I thought would happen apart form the oddity, but yeah hopefully you agree but the less errors a parser throws the better, it's for another project downstream to catch these issues when it consumes the AST.

Talking of errors, any news on the interpolation issues? :innocent:

haoqunjiang commented 8 years ago

After carefully reading the css syntax spec, I have to agree that the aforementioned code block is valid css syntax, though it doesn't make any sense.

But when it comes to less and scss, neither of them could parse this code block (tested in http://lesscss.org/less-preview/ and http://www.sassmeister.com/). I'm not sure if it is a good idea to deviate from the official parser implementation.

DanPurdy commented 8 years ago

Agreed but at the same time, a parser throwing an error causes issues downstream for everyone consuming it, and would mean the rest of that file wouldn't be parsed.

IMO the parser should just be interpreting what it finds into a tree and then a linter or a code checking tool should be flagging that issue up. That code in scss would be wrong for no semicolons but in Sass it would be wrong for having curly braces so it's about striking a balance. It's not really the parsers job to ensure what you have typed is valid and I think that may be where some of the issues we see at the moment are coming from.

The parser that eslint uses for javascript for example follows a similar path and just reports what it finds, eslint then flags up the missing semicolons etc. I guess a halfway house is if the parser output each issue as it found them rather than just throwing on the first one it encounters.

TLDR: it would be nice if these kind of errors were just reported rather than throws so a downstream tool could interpret what was going on rather than just catching an error.

Hope that makes sense? :smile:

haoqunjiang commented 8 years ago

Yes, that makes total sense. Never realized that before… Thanks for your detailed explanation @DanPurdy