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

Improve CSScomb support #623

Merged
merged 1 commit into from
Oct 30, 2015
Merged

Improve CSScomb support #623

merged 1 commit into from
Oct 30, 2015

Conversation

danielbayley
Copy link
Contributor

Add setting for custom config file path and support for CSON in addition to the standard JSON config.

Signed-off-by: Daniel Bayley [email protected]

@danielbayley
Copy link
Contributor Author

Following on from #209

@danielbayley
Copy link
Contributor Author

@Glavin001 Not sure why this is failing? I don't think it has anything to do with the code I added…

LESS: false
Sass: false
SCSS: false
CSS: true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change:

    CSS: true
    LESS: true
    Sass: true
    SCSS: true

to:

    _:
      configPath: true
      predefinedConfig: true
    CSS: true
    LESS: true
    Sass: true
    SCSS: true

Similar to https://github.com/danielbayley/atom-beautify/blob/master/src/beautifiers/prettydiff.coffee#L6

This allows only the options that are supported (configPath and predefinedConfig) and puts that for the default (_).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the last thing that needs fixing. Right now it looks like all options are available, when it should be only configPath and predefinedConfig option. After that, re-run coffee docs/ to generate the correct documentation with only the allowed options. Thanks!

@Glavin001
Copy link
Owner

I usually ignore passing AppVeyor since Windows can be a PITA.

It is currently passing Travis CI, which is what I require before merging Pull Requests, so this is good 👍.

After you fix the above recommended changes, please run coffee docs/ as well which will generate the documentation with the new options you have added.

This is great. Good work so far! Will be able to merge this soon. Thanks.

@danielbayley
Copy link
Contributor Author

CSON.readFileSync seems to actually read JSON too; so Iv'e simplified the logic to take that into account…

since Windows can be a PITA.

Can be?!

@Glavin001
Copy link
Owner

CSON.readFileSync seems to actually read JSON too; so Iv'e simplified the logic to take that into account…

Great!

Add setting for custom config file path and support for CSON in addition to the standard JSON config.

Signed-off-by: Daniel Bayley <[email protected]>
@danielbayley
Copy link
Contributor Author

All sorted now as far as I can tell. The CI build is failing but since I'm getting deprecation warnings that include this package (since this last update to Atom itself) I'm guessing it's something to do with that…

@Glavin001
Copy link
Owner

It's actually failing because of the Titanium Style Sheets tests.
I'm going to merge this and take a look on my own.

Glavin001 added a commit that referenced this pull request Oct 30, 2015
@Glavin001 Glavin001 merged commit 266ad49 into Glavin001:master Oct 30, 2015
@Glavin001
Copy link
Owner

Thank you!

@Glavin001
Copy link
Owner

Published to v0.28.17

@Glavin001 Glavin001 mentioned this pull request Jan 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants