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

Add cacheable config #401

Merged
merged 10 commits into from
Nov 20, 2015
Merged

Conversation

DanPurdy
Copy link
Member

By default the config is now cached on each invocation of sass-lint with an option to override this behaviour.

You can override this behaviour if you want, an example would be someone tweaking their sass-lint config within a IDE plugin.

Updated the tests to work with this new implementation.

Some of the crude benchmarks I ran for loading the config when cached showed an average of a 26% speed increase loading each rule as opposed to always loading from scratch though i'm sure with tweaks we could improve this and definitely an opportunity with 2.0

Looking for feedback here about if everyone is happy for this to move forward. It follows our usual pattern of preference i.e. options > user config > defaults

closes #279

DCO 1.1 Signed-off-by: Dan Purdy <danjpurdy@gmail.com>

@DanPurdy
Copy link
Member Author

Also closes #403

@DanPurdy
Copy link
Member Author

Ready for review now

@benthemonkey
Copy link
Member

A little bit off-topic, but do we need the devDependency on the deep-equal module? My understanding is that the reason to use it over assert.deepEqual is to return a boolean instead of throwing an error. However, we only use it in tests, where throwing is fine.

@benthemonkey
Copy link
Member

Other than that, LGTM. Added a console.log to the cached config condition in config.js and verified that the branch is being hit instead of loading the config again.

@DanPurdy
Copy link
Member Author

Thanks @benthemonkey, I've tried to cover every eventuality with the tests and they're actually doing something now so good catch there! @bgriffith and @Snugug should probably look this over before we merge it as it's quite far reaching in what it does.

As for deep equal, correct me if i'm wrong but we'd need to use the assert.deepStrictEqual method which I 'believe' isn't supported in earlier versions of Node that we test against still? I don't mind removing if we need to though.

@benthemonkey
Copy link
Member

Oops, you're right, 0.10.x doesn't support assert.deepStrictEqual.

@Snugug
Copy link
Member

Snugug commented Nov 16, 2015

In general I like this rule. Think it should be off by default as it's one of those things that can potentially be a gotcha bug that's hard to track down.

@DanPurdy
Copy link
Member Author

Ok, i'll update this so it isn't enabled by default then, I think that does make sense.

@DanPurdy
Copy link
Member Author

Ok updated the defaults to false now. Ready for final review.

@benthemonkey
Copy link
Member

This LGTM

@DanPurdy
Copy link
Member Author

Feel free to merge it, I just wanted the other guys to give it a once over before we merged it in the first place. All good now though.

'no-duplicate-properties': 0,
'indentation': [
2,
{
Copy link
Member

Choose a reason for hiding this comment

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

Funky indentation?

@DanPurdy
Copy link
Member Author

Turns out the indentation was fine @bgriffith, though it looks wrong to me!

@bgriffith
Copy link
Member

Sorry about that! :shipit:

bgriffith added a commit that referenced this pull request Nov 20, 2015
@bgriffith bgriffith merged commit 728c07f into sasstools:develop Nov 20, 2015
@DanPurdy DanPurdy deleted the feature/cache-config branch December 23, 2015 22:17
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.

Cache contents of config file
4 participants