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

Create first config #1

Merged
merged 1 commit into from Sep 24, 2019
Merged

Create first config #1

merged 1 commit into from Sep 24, 2019

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Sep 23, 2019

index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the create/first-config branch 2 times, most recently from 0e3b699 to 5dec396 Compare September 23, 2019 16:48
@skjnldsv skjnldsv added enhancement New feature or request help wanted Extra attention is needed labels Sep 23, 2019
@nickvergessen
Copy link
Contributor

So after this is in, how can I get it into nextcloud/notifications#432 ?

@skjnldsv
Copy link
Contributor Author

So after this is in, how can I get it into nextcloud/notifications#432 ?

See https://github.com/skjnldsv/vueexample/pull/249/files :)

@skjnldsv skjnldsv merged commit 9bf2920 into master Sep 24, 2019
@delete-merged-branch delete-merged-branch bot deleted the create/first-config branch September 24, 2019 11:10
@georgehrke
Copy link

@skjnldsv-bot publish npm

@skjnldsv
Copy link
Contributor Author

@georgehrke Still a few things to finish, the peerdependencies mostly, to avoid other apps updating their packages if the config does not support it

@korelstar
Copy link
Contributor

Hi everybody! A common config is good, but why did you created a new repository for this? We have already https://github.com/nextcloud/eslint-plugin-nextcloud which could be enhanced with a new config!?

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Sep 24, 2019

This is very different.
A plugin provide rules with methods to properly check against.
This is just a config set. Same goes for eslint-config-standard and eslint-plugin-standard

@korelstar
Copy link
Contributor

korelstar commented Sep 24, 2019

eslint-plugin-nextcloud contains a config set, too: https://github.com/nextcloud/eslint-plugin-nextcloud/tree/master/lib/configs

I still don't see, why this needs to be separated.

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Sep 24, 2019

But a plugin only depends on its own configs :)
see https://eslint.org/docs/developer-guide/shareable-configs

@korelstar
Copy link
Contributor

korelstar commented Sep 24, 2019

You can use those configs directly from your custom app. That's what I'm doing in the notes app: https://github.com/nextcloud/notes/blob/master/.eslintrc.js#L22

We can move all that stuff to a new config inside of eslint-plugin-nextcloud and use this config in every app.

@skjnldsv
Copy link
Contributor Author

Looks like a very bad idea to me.
the eslint-config-nextcloud also implements custom dependencies for webpack and vuejs. the esling plugin is a standalone. This should not be mixed.

If we do that, suddenly we get into a very messy dependency management where we have the config requiring webpack, eslint-vue... deps that are not required at all by the plugin (originally).

@skjnldsv
Copy link
Contributor Author

All working fine @georgehrke

$ npm i --save-dev eslint@6.0.0                  
npm WARN eslint-config-nextcloud@0.0.5 requires a peer of eslint@^5.0.0 but none is installed. You must install peer dependencies yourself.

@korelstar any reason you seems to be against this new package?

@korelstar
Copy link
Contributor

I just didn't see the advantage for a separate package (which just means more maintainance overhead). Furthermore, the current implementation contains definitions, we already have in eslint-plugin-nextcloud (e.g. globals).

@skjnldsv
Copy link
Contributor Author

Well, I follow eslint guidelines and don't want to pollute the main plugin which is supposed to provide dedicated nextcloud rules.
Ho wis that more maintenance? It is cleaned up on your apps, less packages, you get assurance of compatibility since dependabot won't suggest updates as long as our eslint package don't support it.

All I see is less thing to do for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants