Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Grammars scopes to config + added support to styled-components #322

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mgtitimoli
Copy link

@mgtitimoli mgtitimoli commented Feb 17, 2017

Hi there!

I've been using Styled Components for a while, and recently we decided to add stylelint to our validation pipeline.

In addition to lint from the console, I wanted to get some feedback directly in Atom, and was then when I found this package.

I installed the package, but to my surprise I was not getting any feedback, so after digging a while I noticed that this was caused because the grammar for Styled Components was not included in the currently supported ones.

In order to add a new grammar there were 2 paths to take:

  1. Add the new grammar directly to the baseScopes array
  2. Promote supported grammars to be a config value, so this way users would be able to specify the grammars they want to lint

After analyzing the pros and cons I ended up picking the nice and marvelous path 2 ✨.

Hope you like this 🙂

By moving Grammars to config we are now allowing users to specify in which grammars should stylelint run.
Add stylelint-processor-styled-components dependency to allow testing for Styled Components files.
@Arcanemagus
Copy link
Member

Looking this over now.

],
"items": {
"type": "string",
"enum": [
Copy link
Member

@Arcanemagus Arcanemagus Feb 22, 2017

Choose a reason for hiding this comment

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

Wow, although this works, it leads to some incredibly broken user experiences. Typing anything other than this allowed list in there results in that non-allowed item disappearing with no warning... this includes accidentally editing one of the allowed list.

There are two approaches that can be taken here:

  • Have a predefined list of scopes, with individual options controlling each group (language), similar to how c9da67a worked.
  • Allow any scopes to be entered, similar to how linter-eslint works (1, 2)

Since this package currently can only use the embedded stylelint, I would say that the first option is the better way to go for now as extra processors currently can't be added by the user (without editing the source code)

tl;dr: Change this whole option to an "Enable Styled Components Support" option 😛.

@@ -76,6 +68,9 @@ export function activate() {
subscriptions.add(atom.config.observe('linter-stylelint.showIgnored', (value) => {
showIgnored = value;
}));
subscriptions.add(atom.config.observe('linter-stylelint.grammarScopes', (value) => {
grammarScopes = value;
Copy link
Member

Choose a reason for hiding this comment

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

This setting needs to be removed, but just for your reference the implementation here wouldn't have worked anyway.

A reference to the initial grammarScopes down in the return from provideLinter is the only one that is ever used, so you would have needed to update the current Array not replace it as you are doing here.

@@ -0,0 +1,9 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Do these import lines need to be in here? They are causing ESLint to blow up CI.


describe('works with JS files (language-babel) with Styled Components and', () => {
beforeEach(() => {
atom.config.set('linter-stylelint.disableWhenNoConfig', false);
Copy link
Member

Choose a reason for hiding this comment

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

There is a configuration for those files, is there any reason you are disabling this setting?

describe('works with JS files (language-babel) with Styled Components and', () => {
beforeEach(() => {
atom.config.set('linter-stylelint.disableWhenNoConfig', false);
waitsForPromise(() => atom.packages.activatePackage('language-babel'));
Copy link
Member

Choose a reason for hiding this comment

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

Does this need language-babel? I'd like to avoid bringing that in here if possible. If it is required, then it needs to be added to the list of pre-installed packages in the CI scripts.

});

it('correctly detects an error present in a file', () => {
const ntscMessage = 'Expected a trailing semicolon (<a href="http://stylelint.io/user-guide/rules/declaration-block-trailing-semicolon">declaration-block-trailing-semicolon</a>)';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why ESLint isn't blowing up on this line being too long already, but can you split this into two lines?

@Arcanemagus
Copy link
Member

@mgtitimoli Are you interested in continuing work on this?

@mgtitimoli
Copy link
Author

Hi @Arcanemagus,

Sorry for taking too long to answer, I will get into this during the day.

@bcotrim
Copy link

bcotrim commented Apr 14, 2017

I would find this useful for my projects.
@mgtitimoli are you going to finish this up? otherwise I could pick it up.

@zaaack
Copy link

zaaack commented Jun 30, 2017

Any updates?

@konstantin24121
Copy link

Anybody complete this PR. Sadly lose linting in Atom with styled-components

@Arcanemagus
Copy link
Member

This PR is now rather out of date, I'll be re-implementing this after the other open PR's are finished up if nobody beats me to it 😉.

@Ky6uk
Copy link

Ky6uk commented May 23, 2018

I found there is no way to lint styled components through this linter yet. Isn't there?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants