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

New rule: space around operator #124

Closed
wants to merge 1 commit into from

Conversation

skovhus
Copy link
Contributor

@skovhus skovhus commented Sep 6, 2015

This resolves #31.

Due to bugs in gonzales-pe the following will currently fail:

body {
  background: url('a'  +  'b.png');  // will not throw an error
}

$foo: 90px   %10px;  // will crash

DCO 1.1 Signed-off-by: Kenneth Skovhus [email protected]

@skovhus skovhus changed the title 🔍 space around operator New rule: space around operator Sep 6, 2015
};

module.exports = {
'name': 'space-around-colon',
Copy link
Member

Choose a reason for hiding this comment

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

Should be space-around-operator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bgriffith Good catch. Should be fixed now.

@skovhus skovhus force-pushed the space-around-operator branch from 0fbccdc to 998cd76 Compare September 6, 2015 15:10
@DanPurdy DanPurdy added this to the 1.3.0 milestone Sep 6, 2015
@DanPurdy
Copy link
Member

DanPurdy commented Sep 6, 2015

Hey @skovhus thanks for this, we'll leave it on review until 1.2.0 is out. I believe we are planning to move to using a develop branch post 1.2 to enable us to simultaneously release hot fixes to master if necessary while continuing to develop and merge rules and features.

As you pointed out too we're awaiting a new release of gonzales-pe to fix some of the issues we've encountered.

@skovhus
Copy link
Contributor Author

skovhus commented Sep 6, 2015

@DanPurdy makes sense. Looking forward to 1.2.0. After that https://github.com/skovhus/SublimeLinter-contrib-sass-lint should be ready for use. : )

Out of curiosity, what were the considerations in choosing gonzales-pe? I haven't looked in to this, but I for me it would make sense to use the same parser/AST as node-sass. That should be pretty battle tested by now.

@DanPurdy
Copy link
Member

DanPurdy commented Sep 6, 2015

Yeah it's gonna be good to get 1.2.0 out, and a sublime linter plugin ready to go is also great news! 👍

I've only come on board recently as I'm looking forward to moving away from the ruby dependencies in some of our projects. @Snugug has talked about it in a few of the earlier issues though., but he will be able to tell you why exactly.

@skovhus skovhus force-pushed the space-around-operator branch from 998cd76 to dcd16ae Compare September 6, 2015 18:30
@Snugug
Copy link
Member

Snugug commented Sep 6, 2015

Gonzales was chosen because Libsass (the compiler that Node Sass uses) doesn't provide its AST publicly and, in its current state, wouldn't be useable enough for linting.

There is an issue in Libsass to expose the AST, but until that lands (and the AST is robust enough to support writing a linter against), we can't use it for Sass Lint.

@skovhus
Copy link
Contributor Author

skovhus commented Sep 6, 2015

@Snugug Thanks for clarifying.

Another thing now that we are talking about the background of this project: what are your vision in terms of rules porting? Do you imagine sass-lint to port all scss-lint rules and their many options?

@Snugug
Copy link
Member

Snugug commented Sep 6, 2015

If you want to discuss this, please open an issue to discuss roadmap

On Sep 6, 2015, at 4:22 PM, Kenneth Skovhus [email protected] wrote:

@Snugug Thanks for clarifying.

Another thing now that we are talking about the background of this project: what are your vision in terms of rules porting? Do you imagine sass-lint to port all scss-lint rules and their many options?


Reply to this email directly or view it on GitHub.

@Snugug
Copy link
Member

Snugug commented Sep 7, 2015

This will need to be re-rolled off of the develop branch

@DanPurdy
Copy link
Member

Hey @skovhus this is ready for a new PR to be created in to develop if you have the time.

Thanks!

@DanPurdy
Copy link
Member

I'm going to close this for now. I've just run through it to update with sass tests and even a few of the scss tests aren't doing what they are supposed to. Will definitely need more work.

@DanPurdy DanPurdy closed this Sep 30, 2015
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.

4 participants