-
-
Notifications
You must be signed in to change notification settings - Fork 696
Simplify configuration #17
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
Conversation
b829e90 to
a8c1dad
Compare
Could you please review this PR in your spare time @mysticatea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR!
Mostly looks good to me. I write some points.
.eslintrc.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vue seems to use plugin:vue-libs/recommended: .eslintrc of vuejs/vue. I think good to follow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, true. This might be a good idea 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think good if we use 8 instead of 7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add 8 additionally. These tests run pretty fast and we'll be sure it's actually bulletproof.
config/base.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I dislike browser environment because this environment includes many global variables which has general names: name, length, status, event, open, parent, etc.... It will possibly cause the problems about no-undef / no-unused-vars / no-shadow rules.
So I prefer that it defines fewer global variables than browser environment manually (then I use window.name instead of name) (browser.js is my config for frontend. This defines classes and whitelisted variables of browser environment).
However, I know I'm in the minority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep this simple 'browser' setting for now, at least for v3.0.0-beta to get a better picture about possible issues with this during feedback gathering period. WDYT @mysticatea ?
I addressed your comments @mysticatea. If it's ok I can merge this and release v3.0.0-beta finally.
Oh, .eslintrc without extensions has been deprecated. Please use an extension.
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today, ESLint 4.0.0 has been released. I'd like to include 4.x in peerDependencies to avoid installation failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. peerDependencies updated
I also added package-lock.json generated by npm 5.
@mysticatea Please approve if you agree with my latest updates and I'll release beta. I removed recommendations for html-end-tags and no-duplicate-attributes because of those slight problems with parser we were discussing. We'll mark them recommended as soon as your new parser fixes mentioned issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you so much!
...d-contributing-templates ASA-194: Add templates for bugs, feature requests and PRs
Uh oh!
There was an error while loading. Please reload this page.
General purpose of this PR is to clean and simplify current code base to get a solid ground for upcoming
v3.0.0-betarelease.Things, that I'm working on in this PR:
update-rulesscriptjsx-uses-varsrule, that was previously available ineslint-plugin-vue(+ documentation and tests)I intentionally removed mysticatea's config from
.eslintrcso that we can make a deliberate decision about preferred styleguide in order to make this plugin's style match other internal projects. I think we should use something more general, likeairbnb-base. But I'd like to hear your thoughts as well @chrisvfritz @mysticatea @yyx990803