Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Makes themes resilient to stylesheets without trailing newlines. #8594

Merged
merged 1 commit into from Jul 30, 2014

Conversation

dangoor
Copy link
Contributor

@dangoor dangoor commented Jul 30, 2014

Also deletes a bit of unused cruft from the dark theme.

For #8490

cc @MiguelCastillo

Also deletes a bit of unused cruft from the dark theme.
@dangoor
Copy link
Contributor Author

dangoor commented Jul 30, 2014

@MiguelCastillo Try to make comments from the "Files changed" tab of the PR rather than the commit. It keeps everything together better and makes the email from GitHub more useful.

The bug is not actually in the LESS parser... it's a problem in our code.

The reason I needed to add the newline in this particular place... consider the last line of Light theme's main.less:

// This is the default theme and doesn't need to do anything!

Without the newline, you end up with:

// This is the default theme and doesn't need to do anything!}

See? The closing brace is at the end of a comment. Adding the newline before the closing brace ensures that this can't happen.

@MiguelCastillo
Copy link
Contributor

@dangoor Yeah, sorry about that. I did it from commits tab as I was quickly going through the changes... But I see what you mean. Looks good, ship it!

dangoor added a commit that referenced this pull request Jul 30, 2014
…ines

Makes themes resilient to stylesheets without trailing newlines.
@dangoor dangoor merged commit a82c939 into master Jul 30, 2014
@dangoor dangoor deleted the dangoor/8490-themes-trailing-newlines branch July 30, 2014 17:36
@marcelgerber
Copy link
Contributor

@dangoor Shouldn't this get into release branch? (So it doesn't trip up theme authors)

@peterflynn
Copy link
Member

@SAplayer We could probably just document it at https://github.com/adobe/brackets/wiki/Creating-Themes for now...

@peterflynn
Copy link
Member

But OTOH this does seem quite safe to put into release

@dangoor
Copy link
Contributor Author

dangoor commented Jul 30, 2014

This would be safe to put into release, but it's also pretty minor. It's not so much a problem with missing trailing newlines as it is a problem with putting a line comment as the last line of the file. For example,

.foo {
    border: 1px solid fuschia;
}

With no newline at the end of that, it should still be okay because it'll just get a second brace added to that last line. I would think that putting a line comment as the very last thing in the file wouldn't be common.

@dangoor
Copy link
Contributor Author

dangoor commented Jul 30, 2014

Thanks for adding the notes to the creating themes document, @peterflynn. I don't think this fix is worth pushing to release (the next release will come soon enough!)

@marcelgerber
Copy link
Contributor

@dangoor Well, your comment made me think about an even more uncommon, but still possible issue:
An unclosed block comment

It will then look like this:

.foo {
  foo: #baa;
}
/*
This block comment breaks everything, yay!
}

@dangoor
Copy link
Contributor Author

dangoor commented Jul 30, 2014

Unclosed block comments are a problem in pretty much any language ;)

@marcelgerber
Copy link
Contributor

@dangoor Yes, but in this particular case, it's unexpected since the original doc (everything except the last line) is valid...

@peterflynn
Copy link
Member

@SAplayer I'm not sure I follow... any broken LESS file will not work, but why is that unexpected? It wouldn't work in a standalone context either. The issue in #8490 is that the LESS code would be perfectly valid normally, but unexpectedly did not work when used in Themes.

@marcelgerber
Copy link
Contributor

Oh well, I just noticed I talked bullshit... I blindly assumed the LESS compiler would compile unclosed block comments (and therefore, such files are valid), but after testing it, I now know it results in an error.
Sorry for the inconvenience.

@peterflynn
Copy link
Member

Heh, no worries!

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

Successfully merging this pull request may close these issues.

None yet

4 participants