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

bad formatting of .less files using @variable or &:hover syntax #489

Closed
guymguym opened this issue Jul 9, 2014 · 14 comments
Closed

bad formatting of .less files using @variable or &:hover syntax #489

guymguym opened this issue Jul 9, 2014 · 14 comments

Comments

@guymguym
Copy link
Contributor

guymguym commented Jul 9, 2014

@evocateur - This commit f416b0b seem to break the handling of .less files using @variable or &:hover formatting.

When @var appears it does very weird things - removes space before the @, add unwanted newline, and more.
With &:hover it injects a space before the : which breaks the pseudo class that was intended.

This issue makes it completely unusable for .less files :(
Any idea how to overcome?
Thanks!

@guymguym guymguym changed the title bad formatting of .less files using @variable or &:hover bad formatting of .less files using @variable or &:hover syntax Jul 9, 2014
@evocateur
Copy link
Contributor

Right now, beautifying LESS (and SASS, or any other flavor of CSS preprocessor) is a bit out of the scope of the js-beautify project. Could you provide us with inputs and expected outputs, at least? Thanks!

@guymguym
Copy link
Contributor Author

guymguym commented Jul 9, 2014

sure,
this is input, which is also the expected output (was the output of previous version):

@well-bg: @bg-color;
@well-border: @border-color;
.well {
    &:hover:not(.disabled):not(.active) {
        box-shadow: 0 1px 1px @border-color, 1px 0 1px @border-color;
    }
}

this is the current output:

@well-bg: @bg-color;

@well-border: @border-color;

.well {
    &: hover: not(.disabled): not(.active) {
        box-shadow: 0 1px 1px@border-color, 1px 0 1px@border-color;

    }
}

@evocateur
Copy link
Contributor

@guymguym Thanks, I'll try to take a stab at this soon. Pull requests welcome and encouraged!

@mpavel
Copy link

mpavel commented Jul 11, 2014

Experiencing the same issue (I'm using this with the Sublime HTMLPrettify). Some example I/O

.entry {
        display: block;
        width: @width;
        height: @height;
        position: absolute;
...

gets modified to:

.entry {
        display: block;
        width: @width;
        height: @height;

position: absolute;
...

Or:

&-4 {
    top: 26.2rem;
    left: @second-left;
    padding-left: 6.5rem;
}

to:

&-4 {
    top: 26.2rem;
    left: @second-left;

padding-left: 6.5rem;
}

@guymguym
Copy link
Contributor Author

@evocateur Thanks. I went over the code and there are few issues.

The @ case:

In css syntax @ can only appear as the first word in a rule (such as @media). For less-css, this is used for variable definition/usage. So here is a proposed fix for the relevant code:

            } else if (ch === '@') {
                // pass along the space we found as a separate item
                if (isAfterSpace) {
                    print.singleSpace();
                }
                output.push(ch);

                // strip trailing space, if present, for hash property checks
                var atRule = peekString(" ").replace(/\s$/, '');
                ...

where the new function peekString() is this:

        function peekString(endChar) {
            var str = eatString(endChar);
            pos -= str.length;
            next();
            return str;
        }

Side question - why does this project has both python and js implementation of the same logic?

@guymguym
Copy link
Contributor Author

The : case

In css syntax pseudo classes/elements can only appear on top level. In less-css they can appear at any level. The only reason the formatter needs to handle : characters is to be able to force a since space after the colon in width: 1px versus no space as in button:hover { ... }.

Currently the code maintains the insideRule variable to handle this, but for less-css this notion is confusing since a new class can appear at any level. Another point to consider in less-css is that a rule can also appear at top-most level for variable definitions as in @bg: red;.

I guess the only way to identify the case correctly is to read the entire line, and detect if it ends with ; or with {. Unless you have a better suggestion.

@guymguym
Copy link
Contributor Author

@evocateur I wonder - would it be useful if the prettify code will start by splitting the entire source text to statements by the characters {, }, ; and only then walk through the content of each statement separately?
The benefit would be that it would be much clearer what type of statement is currently being processed - if the statement ends with { then it is a selector, if it ends with ; then it is a rule. I wouldn't solve all the worlds issues, but would make the ones here a bit easier to tackle.

@mcdonnelldean
Copy link

Same issue using Atom.io. Samples and raw text for testing are in the link: Glavin001/atom-beautify#52

guymguym referenced this issue in guymguym/js-beautify Jul 29, 2014
@guymguym
Copy link
Contributor Author

@evocateur Would you please review my pull request to fix the @ case?
Thanks!
Guy

@vedraan
Copy link

vedraan commented Aug 10, 2014

I have recently switched from Sublime Text 2 to Sublime Text 3. I was using this plugin all the time with LESS and from what I can remember, I never had this issue in ST2. So the later version must have introduced this bug which makes it almost useless for LESS development since you almost always have stuff like @ and :

Hope you guys will be able to review the pull request(s) here and figure it out. It's a great beautify project, best one out there IMO.

@bitwiseman
Copy link
Member

@guymguym - Requesting a review the way you are is fruitless. I didn't even know you were asking for one. Please submit a pull request with tests and python port.

@guymguym
Copy link
Contributor Author

guymguym commented Oct 1, 2014

Hi @bitwiseman.

I submitted a pull request two months ago, but it was indeed without tests and python port which I simply wasn't aware of because this project's code is new to me. Was it fruitless to discuss this here to get to know the project parts for a first time contributor? Is there a better way to do so?

Anyway I just updated my pending pull request with tests and also the python code, and it passed the travis build, so I guess it's ready to pull - #496

However this issue is still not resolved by this fix - the harder problem is with the : case as I mentioned above, and without addressing it this still makes the css beautifier unusable for less-css. In my local setup I just hack this case to avoid beautifying around colons altogether to be able to use it. I will be happy to keep this discussion and think of the best path to resolve.

Cheers

@bitwiseman
Copy link
Member

Ah, I see it. Thanks for you work on this.
I remember now why I didn't merge it - don't know enough to evaluate it's correctness. 😄

Could you squash your commits into one commit and point me to some documentation as to the correct syntax in this case?

As you've noticed, the beautifier is really lacking in LESS support. On one hand that is by design - it's a CSS beautifier not a LESS beautifier. But it seems like no one works in CSS alone.

@bitwiseman bitwiseman added this to the v1.5.3 milestone Oct 1, 2014
@bitwiseman
Copy link
Member

I took your code amended it and made further changes. The fixes are now live in v1.5.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants