Skip to content

Conversation

@SimenB
Copy link
Contributor

@SimenB SimenB commented Feb 23, 2015

Fix #3
Renamed the file to .csslintrc. Is that okay?

@SimenB
Copy link
Contributor Author

SimenB commented Mar 31, 2015

@lazd ping 😄

Copy link
Owner

Choose a reason for hiding this comment

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

@lazd
Copy link
Owner

lazd commented Mar 31, 2015

Looks like this will require some updates to README.md to document how to use this.

I'm +1 to drop passing a string path to the .csslintrc as this replaces that requirement nicely, but I think it might be wise to let folks pass the configuration directly as an object. Thoughts?

@SimenB
Copy link
Contributor Author

SimenB commented Mar 31, 2015

Hmm, I agree that should be possible, but isn't it? There's a unit-test for it.

Although when I look at it, the linline rule is the same as the one in the rc file. Faulty test, I'll try chaning up one of the rules.

EDIT: @lazd if you look in the should support options-test, you'll see it's now using a different rule altogether. It still supports, or am I missing something?
If you agree, I'll update the README as well 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Options should still work

@SimenB
Copy link
Contributor Author

SimenB commented Mar 31, 2015

@lazd Rcloader supports both string and object with options: https://www.npmjs.com/package/rcloader#options

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I see options is passed to RcLoader here, nice.

@lazd
Copy link
Owner

lazd commented Mar 31, 2015

@SimenB I see my concern wasn't valid, you were already passing options to RcLoader :)

After the README is in shape, I think we're good to merge. Thanks!

@SimenB
Copy link
Contributor Author

SimenB commented Mar 31, 2015

@lazd Something like that? I'm unsure how to formulate it 😆

Copy link
Owner

Choose a reason for hiding this comment

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

Nice one, most people blow away that whitespace and it makes me sad :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's why I usually add it to PRs when making them. IDEs really enjoy strpping that whitespace...

@lazd
Copy link
Owner

lazd commented Mar 31, 2015

I think the README updates you added are good enough to merge, I can re-arrange it after merge.

Let's figure out the best way to handle lookup: false and merge this!

@SimenB
Copy link
Contributor Author

SimenB commented Mar 31, 2015

lookup: false comes from rcloader, not sure how to handle it. As long as it's not introduced in a version of csslint covered by ^0.10.0 we should be good. Maybe lock it down? Then if there ever is a new release of csslint you can check if there is any rule called lookup?

@lazd
Copy link
Owner

lazd commented Mar 31, 2015

I think we're fine with lookup: false as is if it's being handled by RcLoader. I think we're good to go!

lazd added a commit that referenced this pull request Mar 31, 2015
Automatically lookup .ccslintrc
@lazd lazd merged commit 32838e7 into lazd:master Mar 31, 2015
@SimenB SimenB deleted the auto-find-csslintrc branch March 31, 2015 16:46
@SimenB
Copy link
Contributor Author

SimenB commented Mar 31, 2015

Great, thanks!

@SimenB
Copy link
Contributor Author

SimenB commented Mar 31, 2015

@lazd What's missing for a 1.0.0 release? #1?

@SimenB
Copy link
Contributor Author

SimenB commented Apr 24, 2015

@lazd Could you npm publish this?

@lazd
Copy link
Owner

lazd commented Apr 24, 2015

@SimenB merging a couple other PRs and I'll get a release out today!

@SimenB
Copy link
Contributor Author

SimenB commented Apr 24, 2015

Awesome 😄

@lazd
Copy link
Owner

lazd commented Apr 24, 2015

Hey @SimenB, the README is still referencing passing the path to csslintrc.json to the plugin. We discussed dropping support for that, what do you think?

@SimenB
Copy link
Contributor Author

SimenB commented Apr 24, 2015

@lazd It's still supported, although it I suppose the recommended way is automatic lookup?

This change should be backwards compatible.

@lazd
Copy link
Owner

lazd commented Apr 24, 2015

Alright, I suppose we can leave it for now. Let's consider dropping it for 1.0.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auto load .csslintrc

2 participants