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

bears/stylus: Add StylintBear #1610

Merged
merged 1 commit into from Jun 4, 2017
Merged

bears/stylus: Add StylintBear #1610

merged 1 commit into from Jun 4, 2017

Conversation

@yash-nisar
Copy link
Member

@yash-nisar yash-nisar commented Apr 16, 2017

Closes #754

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

  • I read the commit guidelines and I've followed
    them.
  • I ran coala over my code locally. (All commits have to pass
    individually.
    It is not sufficient to have "fixup commits" on your PR,
    our bot will still report the issues for the previous commit.) You will
    likely receive a lot of bot comments and build failures if coala does not
    pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Apr 16, 2017

@Makman2 @jayvdb Should we try to include this in the next release ?

Loading

Copy link
Member

@jayvdb jayvdb left a comment

Unlikely to be able to get new bear PRs through code review by the next release. Lots of design work. More likely to get old bear PRs in.

Loading

:param allow_brackets:
When ``always``, expect ``{}`` when declaring a selector. When
``never``, expect no brackets when declaring a selector.
:param allow_colons:
Copy link
Member

@jayvdb jayvdb Apr 16, 2017

Choose a reason for hiding this comment

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

Are there only two options, 'never' and 'always', or a third equivalent "not set". If it is a bi-state or tri-state, use bool.

Loading

Copy link
Member Author

@yash-nisar yash-nisar Apr 16, 2017

Choose a reason for hiding this comment

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

I've used the default settings given here. Should we document the rules like I've documented them or should we directly call return filename, ? I think the current approach would help people who would write config files and might want rules to be manipulated.

Loading

Copy link
Member

@jayvdb jayvdb Apr 26, 2017

Choose a reason for hiding this comment

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

This is not about documentation. is allow_colons bi-state or tri-state. If so, use the bool type with values (True, False, None) instead using a string type.

Loading

Set 'max' number of Errors.
:param set_max_warnings:
Set 'max' number of Warnings.
:param mixed_spaces_and_tabs:
Copy link
Member

@jayvdb jayvdb Apr 16, 2017

Choose a reason for hiding this comment

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

we have standard config variable for this.

We bend the linter to fit into coala's simpler settings system; we do not bend coala bears to suit the linters config names.

Loading

Copy link
Member Author

@yash-nisar yash-nisar Apr 16, 2017

Choose a reason for hiding this comment

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

I completely agree but the behaviour of mixed_spaces_and_tabs changes with the indent_size, so based on the indent_size, which by default is False, it will use tabs. If we define an indent_size, it will use spaces. There are a couple of examples here.

Example if indentPref: 4 and mixed: true: prefer \s\s\s\smargin\s0 over \tmargin\s0
Example if indentPref: 2 and mixed: true: prefer \s\smargin\s0 over \tmargin\s0
Example if indentPref: false and mixed: true: prefer \tmargin\s0 over \s\s\s\smargin\s0

Loading

Copy link
Member

@jayvdb jayvdb Apr 26, 2017

Choose a reason for hiding this comment

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

Find a way to hide their config variables from our users.
Our users want to use the standard config variables that are the same across all linters.
They do not want to understand the complexities of each linter's config.

Loading

check_duplicates: bool=True,
allow_efficient_properties: str='always',
set_extend_preference: bool=False,
check_global_duplicates: bool=False,
Copy link
Member

@jayvdb jayvdb Apr 16, 2017

Choose a reason for hiding this comment

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

why False?

Loading

Copy link
Member Author

@yash-nisar yash-nisar Apr 16, 2017

Choose a reason for hiding this comment

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

I'm discarding the setting because it doesn't make sense 😆

Loading

Pass in either ``@extend`` or ``@extends`` and then enforce that.
Both are valid in Stylus. It doesn't really matter which one
you use.
:param check_global_duplicates:
Copy link
Member

@jayvdb jayvdb Apr 16, 2017

Choose a reason for hiding this comment

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

how will this work on all files when only one file is passed to the linter??

Loading

Copy link
Member Author

@yash-nisar yash-nisar Apr 16, 2017

Choose a reason for hiding this comment

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

How did I miss it ! 😕

Loading

@gitmate-bot
Copy link
Collaborator

@gitmate-bot gitmate-bot commented Apr 23, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Loading

:param allow_brackets:
When ``always``, expect ``{}`` when declaring a selector. When
``never``, expect no brackets when declaring a selector.
:param allow_colons:
Copy link
Member

@jayvdb jayvdb Apr 26, 2017

Choose a reason for hiding this comment

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

This is not about documentation. is allow_colons bi-state or tri-state. If so, use the bool type with values (True, False, None) instead using a string type.

Loading

Set 'max' number of Errors.
:param set_max_warnings:
Set 'max' number of Warnings.
:param mixed_spaces_and_tabs:
Copy link
Member

@jayvdb jayvdb Apr 26, 2017

Choose a reason for hiding this comment

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

Find a way to hide their config variables from our users.
Our users want to use the standard config variables that are the same across all linters.
They do not want to understand the complexities of each linter's config.

Loading

set_max_errors: bool=False,
set_max_warnings: bool=False,
mixed_spaces_and_tabs: bool=False,
set_naming_convention: bool=False,
Copy link
Member

@jayvdb jayvdb Apr 26, 2017

Choose a reason for hiding this comment

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

Loading

mixed_spaces_and_tabs: bool=False,
set_naming_convention: bool=False,
set_strict_naming_convention: bool = False,
allow_none_keyword: str='never',
Copy link
Member

@jayvdb jayvdb Apr 26, 2017

Choose a reason for hiding this comment

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

tristate - use bool

same applies for almost everywhere that you have str='always' or str='never'

Loading

set_max_warnings: bool=False,
mixed_spaces_and_tabs: bool=False,
set_naming_convention: bool=False,
set_strict_naming_convention: bool = False,
Copy link
Member

@jayvdb jayvdb Apr 26, 2017

Choose a reason for hiding this comment

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

undesirable spaces around =

Loading

@@ -0,0 +1,29 @@
import json
Copy link
Collaborator

@gitmate-bot gitmate-bot Apr 26, 2017

Choose a reason for hiding this comment

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

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section flakes.

The issue can be fixed by applying the following patch:

--- a/bears/stylus/StylintBear.py
+++ b/bears/stylus/StylintBear.py
@@ -1,4 +1,3 @@
-import json
 
 from coalib.bearlib.abstractions.Linter import linter
 from dependency_management.requirements.NpmRequirement import NpmRequirement

Loading

@@ -0,0 +1,29 @@
import json
Copy link
Collaborator

@gitmate-bot gitmate-bot Apr 26, 2017

Choose a reason for hiding this comment

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

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section flakes.

The issue can be fixed by applying the following patch:

--- a/bears/stylus/StylintBear.py
+++ b/bears/stylus/StylintBear.py
@@ -1,4 +1,3 @@
-import json
 
 from coalib.bearlib.abstractions.Linter import linter
 from dependency_management.requirements.NpmRequirement import NpmRequirement

Loading

Copy link
Member

@jayvdb jayvdb left a comment

needs tests regarding standard settings, such as tabs&spaces, indents, spaces at end of line, and line length.

Loading

@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Apr 26, 2017

Is the StylintBear.py file good @jayvdb ?

Loading

@gitmate-bot
Copy link
Collaborator

@gitmate-bot gitmate-bot commented May 4, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Loading

3 similar comments
@gitmate-bot
Copy link
Collaborator

@gitmate-bot gitmate-bot commented May 11, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Loading

@gitmate-bot
Copy link
Collaborator

@gitmate-bot gitmate-bot commented May 18, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Loading

@gitmate-bot
Copy link
Collaborator

@gitmate-bot gitmate-bot commented May 18, 2017

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

Loading

"""
Attempts to catch little mistakes (duplication of rules for instance) and
to enforce a code style guide. Like Stylus(a dynamic stylesheet language
that is compiled into CSS) itself, this linter opts for flexibility
Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

Ah wait this linter is for Stylus-files, right? sorry I've misunderstood that. Rather do

Attempts to catch little mistakes (...) and to enforce a code style guide on Stylus .sty files.

Or similar :3 Sorry

Loading

Copy link
Member Author

@yash-nisar yash-nisar May 31, 2017

Choose a reason for hiding this comment

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

Fixed.

Loading

def load_testfile(name):
with open(get_testfile_path(name)) as f:
output = f.readlines()
return output
Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

if you want you can immediately return:

with open(...) as f:
    return f.readlines()

the context manager will exit properly ;)

Loading

Copy link
Member Author

@yash-nisar yash-nisar May 31, 2017

Choose a reason for hiding this comment

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

Fixed.

Loading


@linter(executable='stylint',
output_format='regex',
output_regex=r'(?P<line>\d+):?(?P<column>\d+)?\s+.*'
Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

I assume that the colon only appears together with a column, right?
Then you can do

r'(?P<line>\d+)(?::(?P<column>\d+))?

Loading

Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

\s+.*
why .*? Are the results reported on multiple lines?

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 1, 2017

Choose a reason for hiding this comment

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

Due to the difference in output messages :

  1. For mixed spaces and tabs, I get
3:0 mixed warning mixed spaces and tabs
  1. For trailiing whitespaces, I get
2:13  warning  trailing whitespace  trailingWhitespace

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 1, 2017

Choose a reason for hiding this comment

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

So, to capture the extra mixed, I had to use that exp.

Loading

Copy link
Member

@Makman2 Makman2 Jun 1, 2017

Choose a reason for hiding this comment

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

Ah okay. Make it at least greedy then, so the following warning|error group matches at the first occurrence of those words.
-> .*?

Not that we get such a message:

3:0 mixed warning This is a warning ya know bro

would be parsed to

line=3
column=0
message=ya know bro

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 2, 2017

Choose a reason for hiding this comment

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

Fixed. :)

Loading

r'(?P<severity>error|warning)\s+(?P<message>.+)')
class StylintBear:
"""
Attempts to catch little mistakes (duplication of rules for instance) and
Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

if you have a list of things stylint can fix, you can write them all out :)
e.g.

Attempts to catch little mistakes and and to enforce.......

The `StylintBear` is able to catch following problems:
- Duplication of rules
- ...
- ...

The more docs, the better ;)

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 1, 2017

Choose a reason for hiding this comment

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

Since the list is exhaustive, I'm mentioning some of the problems. Thumbs up if you agree. :)

Loading

Copy link
Member

@Makman2 Makman2 Jun 1, 2017

Choose a reason for hiding this comment

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

Choose appropriate ones reflecting as much of stylint's capabilities as possible ;)

Loading

class StylintBear:
"""
Attempts to catch little mistakes (duplication of rules for instance) and
to enforce a code style guide on Stylus(.sty) files. Like Stylus
Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

Missing space after before (.sty)

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 1, 2017

Choose a reason for hiding this comment

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

Fixed.

Loading

Attempts to catch little mistakes (duplication of rules for instance) and
to enforce a code style guide on Stylus(.sty) files. Like Stylus
(a dynamic stylesheet language that is compiled into CSS) itself, this
linter opts for flexibility over rigidity.
Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

Actually I'm not sure we need this sentence ;) It doesn't provide really useful information imo :)

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 1, 2017

Choose a reason for hiding this comment

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

Fixed.

Loading

"""
:param stylint_config:
The location of the ``.stylintrc`` config file. All the above
options are not used if this option is present, instead the
Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

There are no options above yet

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 1, 2017

Choose a reason for hiding this comment

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

Fixed, will add this bit of info in the next PR where the options will be added.

Loading

.bankTitle
font-weight: bold;
padding-right: 2em

Copy link
Member

@Makman2 Makman2 May 31, 2017

Choose a reason for hiding this comment

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

missing \n at EOF on purpose?

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 1, 2017

Choose a reason for hiding this comment

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

Nope, my bad as always. 😛

Loading

@yash-nisar yash-nisar force-pushed the stylint-bear branch 2 times, most recently from 8852f30 to 80db086 Jun 1, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Jun 1, 2017

Loading

enforces a code style guide on Stylus (a dynamic stylesheet language
with the `.styl` extension that is compiled into CSS) files.
The `StylintBear` is able to catch following problems:
Copy link
Member

@Makman2 Makman2 Jun 1, 2017

Choose a reason for hiding this comment

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

I'm still unsure about using single backticks with rst, could you use double backticks to be safe? Or you can keep using them if you proof me that code highlighting works fine with those too ;)

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 2, 2017

Choose a reason for hiding this comment

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

Fixed. :)

Loading

- Unnecessary brackets
- Missing colon between property and value
- Naming conventions
- Trailing whitespace
Copy link
Member

@Makman2 Makman2 Jun 1, 2017

Choose a reason for hiding this comment

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

some more advanced features maybe too?

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 2, 2017

Choose a reason for hiding this comment

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

Done.

Loading

@yash-nisar yash-nisar force-pushed the stylint-bear branch 3 times, most recently from b7a963c to 09a1197 Jun 2, 2017
@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Jun 2, 2017

Loading

file_contents,
[Result.from_values('StylintBear',
message='missing colon between property '
'and value colons',
Copy link
Member

@Makman2 Makman2 Jun 2, 2017

Choose a reason for hiding this comment

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

hm the message looks bad with this colons at the end. What does it mean anyway?

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 2, 2017

Choose a reason for hiding this comment

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

Maybe the name of the rule.

Loading

Copy link
Member

@Makman2 Makman2 Jun 3, 2017

Choose a reason for hiding this comment

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

could we strip that off somehow?

Loading

Copy link
Member

@Makman2 Makman2 Jun 3, 2017

Choose a reason for hiding this comment

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

it seems if there are exactly two spaces following a word, it's this rule^^

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 3, 2017

Choose a reason for hiding this comment

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

Hmm, maybe we can strip it off after capturing 2 spaces in the end.

Loading

Copy link
Member Author

@yash-nisar yash-nisar Jun 3, 2017

Choose a reason for hiding this comment

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

Will fail for #1610 (comment), so I think we have to keep it as it is.

Loading

Copy link
Member

@Makman2 Makman2 Jun 4, 2017

Choose a reason for hiding this comment

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

That was not so easy, but this works :)

(?P<line>\d+):?(?P<column>\d+)?\s+.*?(?P<severity>error|warning)\s+(?P<message>.+?)(?:  .*|\n|$)

Try to match a message until:

  • Two spaces occur, and complete the match by consuming everything behind 2 spaces
  • A new line occurs
  • The end of string occurs (that might not terminate with \n, that's the reason for this special case)

Loading

@yash-nisar
Copy link
Member Author

@yash-nisar yash-nisar commented Jun 4, 2017

Updated @Makman2

Loading

@Makman2
Copy link
Member

@Makman2 Makman2 commented Jun 4, 2017

ack 47d3956

Loading

@Makman2
Copy link
Member

@Makman2 Makman2 commented Jun 4, 2017

@rultor merge

Loading

@rultor
Copy link

@rultor rultor commented Jun 4, 2017

@rultor merge

@Makman2 OK, I'll try to merge now. You can check the progress of the merge here

Loading

@rultor rultor merged commit 47d3956 into coala:master Jun 4, 2017
6 of 9 checks passed
Loading
@rultor
Copy link

@rultor rultor commented Jun 4, 2017

@rultor merge

@Makman2 Done! FYI, the full log is here (took me 2min)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

5 participants