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

feat: better options default #263

Merged
merged 1 commit into from
Sep 9, 2021
Merged

Conversation

theoludwig
Copy link
Contributor

@theoludwig theoludwig commented Aug 13, 2021

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[x] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)

As explained in #261, this PR reduce configurations overhead for users so that they can use the extension fast without too much configurations and still following "best practices" by encouraging local installation per project.

This PR adds a new option "standard.enableGlobally" (false by default) to enable the extension for all files even if there is no engines installed in package.json in devDependencies.

Which issue (if any) does this pull request address?

Fixes #261

Is there anything you'd like reviewers to focus on?

BREAKING CHANGE: This feature changed the default settings, before: "standard.usePackageJson": false, after: "standard.usePackageJson": true

BREAKING CHANGE: By default (if you don't set "standard.enableGlobally": true), the extension will not lint your files if you haven't got a package.json containing one of the engines installed in devDependencies.

(Easily fixable by editing .vscode/settings.json, long time ago we didn't have BREAKING CHANGE for this extension so that should be fine)

Copy link
Contributor Author

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

I left some comments on why I did certain things.


To do this, set `"javascript.validate.enable": false` in your VSCode `settings.json`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to disable this setting while using this extension, and I would encourage to enable it so you can get both linting and javascript errors.

README.md Outdated

If you don't know how to install extensions in VSCode, take a look at the [documentation](https://code.visualstudio.com/docs/editor/extension-gallery#_browse-and-install-extensions).
Launch VSCode Quick Open (⌘+P), paste the following command, and press enter.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about making easier for the user to install the extension instead of redirecting them to the documentation ?

We're still redirecting them to the documentation if they want more information.


## Plugin options
## Extension options
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it consistent, we use the word Extension instead of Plugin, it means the same thing in this context.

@theoludwig theoludwig requested a review from fmauNeko August 14, 2021 09:34
@theoludwig theoludwig force-pushed the feat/better-options-default branch from d7117fd to 2bf6363 Compare August 15, 2021 22:12
README.md Outdated Show resolved Hide resolved
package.json Outdated
@@ -41,7 +41,7 @@
"standard.enable": {
"scope": "resource",
"type": "boolean",
"default": true,
"default": false,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, won't this turn off standard linting for anyone currently using the extension, when upgrading to this version? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why it is a BREAKING CHANGE.

I don't think it is a great idea to enable standard linting by default globally, it should be enabled by each project individually so if one project use eslint instead of standard engines, it doesn't conflict.

Maybe we can do better ?
We can detect if one of the engines (standard, semistandard, standardx or ts-standard) is installed in devDependencies if so we enable the extension?
That way we don't need any settings in.vscode/settings.json, it just works out of the box (as one of standard goals).
What do you think?

Copy link
Member

@voxpelli voxpelli Aug 16, 2021

Choose a reason for hiding this comment

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

I think there should be three levels:

  1. Globally enabled
  2. Globally disabled
  3. Globally enabled in projects that has a recognizable standard setup

And the default should be the third option in my opinion.

Edit: But I have no knowledge how this is currently set up, just wanted to add my thoughts on the desired state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree too the default should be the third option, so we don't need any settings to use this extension by default. 👍

Currently, the extension is working by default with the first option (Globally enabled), this PR was trying to make it work like the second option (Globally disabled).

When I will have some time, I will update the PR to make it work like the third option.
Thanks for making it clearer! 😄

Copy link
Contributor Author

@theoludwig theoludwig Sep 5, 2021

Choose a reason for hiding this comment

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

Done! I updated the PR.

Also, I added in CHANGELOG.md what changed and bump the version number to 2.0.0, so we can release a new version after this PR has been merged.
See CHANGELOG.md for an explanation about the new behavior.

@theoludwig theoludwig requested a review from LinusU September 5, 2021 20:49
BREAKING CHANGE: This feature changed the default settings, before: `"standard.usePackageJson": false`, after: `"standard.usePackageJson": true`

BREAKING CHANGE: By default (if you don't set `"standard.enableGlobally": true`), the extension will not lint your files if you haven't got a `package.json` containing one of the engines installed in `devDependencies`.
@theoludwig theoludwig force-pushed the feat/better-options-default branch from baab79f to 61dea81 Compare September 9, 2021 09:07
Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

🚀

@theoludwig theoludwig merged commit 776593a into master Sep 9, 2021
@theoludwig theoludwig deleted the feat/better-options-default branch September 9, 2021 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Better options default to reduce configurations overhead
4 participants