Improved config loading allowing more options. #95
Conversation
Swiftwork
commented
Apr 18, 2017
- Added configBasedir and config options.
- Configs can now be loaded from a specified file path or from ".stylelintrc" and "stylelint.config.js".
- Config can also be specified as objects either in vscode settings or in the package.json.
- Removed useStylelintConfigOverrides because it became obsolete.
+ Added configBasedir and config options. + Configs can now be loaded from a specified file path or from ".stylelintrc" and "stylelint.config.js". + Config can also be specified as objects either in vscode settings or in the package.json. - Removed useStylelintConfigOverrides because it became obsolete.
|
I followed your postcss-sorting plugin and vscode-config-resolver pattern ;) |
|
Thanks for your work! 💪 I'll review this PR very soon. |
package.json
Outdated
| @@ -41,10 +41,18 @@ | |||
| "type": "object", | |||
| "title": "stylefmt configuration", | |||
| "properties": { | |||
| "stylefmt.useStylelintConfigOverrides": { | |||
There was a problem hiding this comment.
I think we should revert this changes, because this feature is related to issue #8.
| @@ -75,4 +84,4 @@ | |||
| "build": "npm run clean && npm run lint && npm run compile", | |||
| "watch": "npm run clean && npm run lint && npm run compile -- --sourceMap --watch" | |||
| } | |||
| } | |||
| } | |||
src/extension.ts
Outdated
| editorSettings: settings.config | ||
| }; | ||
|
|
||
| return configResolver.scan(document.uri.fsPath, options).then((resolved) => { |
There was a problem hiding this comment.
Here we should merge resolved config and config from stylelint.configOverrides option if stylefmt.useStylelintConfigOverrides option is enabled.
There was a problem hiding this comment.
Will look into that but sounds reasonable
src/extension.ts
Outdated
| config: resolved.json | ||
| }); | ||
| }).catch(() => { | ||
| return stylefmtProcess(document, range); |
There was a problem hiding this comment.
I think that we shouldn't work without config. This is abnormal situation and we need to inform the user. You can replace this line on showOutput(err.toString());?
There was a problem hiding this comment.
But stylefmt supports usage without config which indicates to me that that is the appropriate behaviour. However it is up to you, else I believe devs should disable the plugin for workspaces where it is not needed.
|
Hello, @Swiftwork, I added a few comments, if you agree with me, then please correct them, if not, then I am ready to discuss them. |
|
I made a new commit with the discussed changes :) |
src/extension.ts
Outdated
| configBasedir: settings.configBasedir || vscode.workspace.rootPath, | ||
| config: resolved.json | ||
| configBasedir: settingsFmt.configBasedir || vscode.workspace.rootPath, | ||
| config: extend(true, {}, resolved.json, { rules: configOverrides || {} }) |
There was a problem hiding this comment.
Please, use Object.assign instead of extend module.
There was a problem hiding this comment.
Issue with Object.assign is that it doesn't deep copy, so nested properties will be lost rather than overwritten. We could write our own extender, but I didn't see the benefit in that.
There was a problem hiding this comment.
Yeap, sorry, i forgot that stylelint has nested config.
|
I'll merge your PR today, but letter 👍 Thanks for your work! |
|
You are welcome, now we just need to exact mimic this behaviour on your postcss-sorting plugin and solve formatOnSave :) |
|
If you want add more features, please create issues or PRs 💃 |
|
If I was unclear, I was referring to another of your vscode plugins. No more additions are planned for this PR and it is ready to merge from my side. |