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/general: Add TextLintBear #1597

Merged
merged 1 commit into from Jun 17, 2017
Merged

Conversation

yash-nisar
Copy link
Member

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

Closes #1576

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!


default_config = {'rules': rules,
"plugins": [
"rst", "html"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not use the preferred quotation marks.

QuotesBear, severity NORMAL, section python.

The issue can be fixed by applying the following patch:

--- a/bears/general/TextLintBear.py
+++ b/bears/general/TextLintBear.py
@@ -88,7 +88,7 @@
 
         default_config = {'rules': rules,
                           "plugins": [
-                              "rst", "html"
+                              "rst", 'html'
                               ]
                           }
         return json.dumps(default_config)


default_config = {'rules': rules,
"plugins": [
"rst", "html"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not use the preferred quotation marks.

QuotesBear, severity NORMAL, section python.

The issue can be fixed by applying the following patch:

--- a/bears/general/TextLintBear.py
+++ b/bears/general/TextLintBear.py
@@ -88,7 +88,7 @@
 
         default_config = {'rules': rules,
                           "plugins": [
-                              "rst", "html"
+                              'rst', "html"
                               ]
                           }
         return json.dumps(default_config)

}

default_config = {'rules': rules,
"plugins": [
Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not use the preferred quotation marks.

QuotesBear, severity NORMAL, section python.

The issue can be fixed by applying the following patch:

--- a/bears/general/TextLintBear.py
+++ b/bears/general/TextLintBear.py
@@ -87,7 +87,7 @@
         }
 
         default_config = {'rules': rules,
-                          "plugins": [
+                          'plugins': [
                               "rst", "html"
                               ]
                           }

Abreviate is a commonly misspelled word.
So should be avoided at the beginning of a sentence.
</body>
</html>
Copy link
Member

Choose a reason for hiding this comment

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

noeol@eof

}

rules['max-number-of-lines'] = {
'max': 300
Copy link
Member

Choose a reason for hiding this comment

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

we have a bear which does this; this bear should accept the same configuration setting name.

trailing comma so future additions dont need to modify this line.

@staticmethod
def generate_config(filename, file):
rules = {
'no-todo': True,
Copy link
Member

Choose a reason for hiding this comment

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

we have a bear which does this? If so, this bear should accept the same configuration setting name.

'date-weekday-mismatch': True,
'no-nfd': True,
'no-surrogate-pair': True,
'ginger': True
Copy link
Member

Choose a reason for hiding this comment

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

trailing comma so future additions dont need to modify this line. (and everywhere else)

rules['unexpanded-acronym'] = {
'min_acronym_len': 3,
'max_acronym_len': 5,
'ignore_acronyms': ['OSS']
Copy link
Member

Choose a reason for hiding this comment

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

we have a bear which does this? If so, this bear should accept the same configuration setting name.

'ignore': []
}

rules['write-good'] = {
Copy link
Member

Choose a reason for hiding this comment

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

isnt this a separate bear? again, this bear would need to match that bear settings.

rules['no-dead-link'] = {
'checkRelative': False,
'baseURI': None,
'ignore': []
Copy link
Member

Choose a reason for hiding this comment

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

we have a bear which does this? If so, this bear should accept the same configuration setting name.

@yash-nisar yash-nisar changed the title bears/general: Add TextLintBear WIP bears/general: Add TextLintBear Apr 9, 2017
@yash-nisar yash-nisar force-pushed the textlint-bear branch 3 times, most recently from 8e35a77 to b9842f1 Compare April 9, 2017 19:25
@yash-nisar
Copy link
Member Author

Aaaaannnddd the testsss passsss 🎉

@gitmate-bot
Copy link
Collaborator

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

1 similar comment
@gitmate-bot
Copy link
Collaborator

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

.travis.yml Outdated
@@ -77,6 +77,7 @@ env:

before_install:
# Remove Ruby directive from Gemfile as this image has 2.2.5
- nvm install 6.10.2
Copy link
Member

Choose a reason for hiding this comment

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

the comment above about Ruby, relates to the line below about Ruby. And in the middle you've added Node ...

Copy link
Member Author

Choose a reason for hiding this comment

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

A very irresponsible act by me.

NpmRequirement('textlint-rule-unexpanded-acronym',
'1.2.1'),
NpmRequirement('textlint-rule-write-good', '1.6.0'),
PipRequirement('docutils-ast-writer', '0.1.2')}
Copy link
Member

Choose a reason for hiding this comment

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

why do we need docutils-ast-writer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should comment on that above the PipRequirement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@gitmate-bot
Copy link
Collaborator

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

7 similar comments
@gitmate-bot
Copy link
Collaborator

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

@gitmate-bot
Copy link
Collaborator

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

@gitmate-bot
Copy link
Collaborator

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

@gitmate-bot
Copy link
Collaborator

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

@gitmate-bot
Copy link
Collaborator

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

@gitmate-bot
Copy link
Collaborator

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

@gitmate-bot
Copy link
Collaborator

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

# Header B

Text.
```
Copy link
Member

Choose a reason for hiding this comment

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

ah RST syntax please :3 (already forgotten that this is not Markdown :P)

... there is an empty section ``#Header A`` below::       <-- double colons

    # Header A

    # Header B

    Text.

Copy link
Member

Choose a reason for hiding this comment

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

maybe you want to shorten the example by one line too:

# Header A

# Header B
Text.

up to you :)

check_common_misspellings: bool=True,
write_good: bool=True,
check_relative_links: bool=False,
base_uri: str='null',
Copy link
Member

Choose a reason for hiding this comment

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

what about leaving the default empty? Though it seems that textlint doesn't like this :3


@linter(executable='textlint',
output_format='regex',
output_regex=r'(?P<line>\d+):(?P<column>\d+)[\s*|✓]*(?P<severity>'
Copy link
Member

Choose a reason for hiding this comment

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

[\s*|checkmark-character] --> (?:\s|checkmark-character)*

[] denotes a character class and is not the same as a group (). Also you don't need the quantifier behind \s if you define it for the group 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Works like a charm. 🎉
regex_edit

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested the unicode manually to make sure that everything works (which is anyway proved above 😛 )
regex_edit


@linter(executable='textlint',
output_format='regex',
output_regex=r'(?P<line>\d+):(?P<column>\d+)[\s*|✓]*(?P<severity>'
Copy link
Member

Choose a reason for hiding this comment

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

I know python supports unicode, though I wouldn't stress that support on different platforms. Is it possible to write the checkmark character as some kind of unicode-code? Like \x0030 or so?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pasted this in the wrong comment box, should be pasted here.

@yash-nisar
Copy link
Member Author

Some changes that I think will be helpful :

@yash-nisar
Copy link
Member Author

Some minor improvements done, @Makman2 your turn to review. 😉

@Makman2
Copy link
Member

Makman2 commented Jun 15, 2017

Replacing check_offensive_expressions with check_with_alex as there are a couple of things that this rule can do apart form checking offensive expressions.

Alright, though the name checking_offensive_expressions was so precise and I liked it :3 But yeah if it can check more than that, it doesn't help :)

r'(?: .*|\n|$)')
class TextLintBear:
"""
The pluggable linting tool for text and markdown. It is similar to
Copy link
Member

Choose a reason for hiding this comment

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

Hm Wikipedia writes markdown uppercase. Let's follow that convention :)

class TextLintBear:
"""
The pluggable linting tool for text and markdown. It is similar to
ESLint, but textlint for natural language.
Copy link
Member

Choose a reason for hiding this comment

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

"but textlint for natural language"? I mean you are just describing TextLint and referring to itself again? :)
Maybe just: It is similar to ESLint, but covers natural language (instead). (The "instead" is optional)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hehe, didn't notice that. Sorry

list of common misspellings.
:param allow_passive_voice:
Allows passive voice.
:param allow_so_beginning:
Copy link
Member

Choose a reason for hiding this comment

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

setting rename:

allow_sentence_beginning_with_so

Copy link
Member Author

Choose a reason for hiding this comment

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

I've followed the standard used in https://github.com/coala/coala-bears/blob/master/bears/natural_language/WriteGoodLintBear.py#L37.

Maybe we should rename both the instances or keep this as it is ?

Copy link
Member

Choose a reason for hiding this comment

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

ah then keep it :)

Allows passive voice.
:param allow_so_beginning:
Allows ``So`` at the beginning of a sentence.
:param allow_adverbs:
Copy link
Member

Choose a reason for hiding this comment

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

you should be more precise with that setting name, as completely removing adverbs in a text is going to make things hard :P

Copy link
Member Author

Choose a reason for hiding this comment

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

I've followed the standard used in https://github.com/coala/coala-bears/blob/master/bears/natural_language/WriteGoodLintBear.py#L38

Maybe we should rename both the instances or keep this as it is ?

Copy link
Member

Choose a reason for hiding this comment

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

keep it then :)

``very``, ``extremely``, etc.
:param allow_repeated_words:
Allows lexical illusions, i.e. cases where a word is repeated.
:param allow_there_is:
Copy link
Member

Choose a reason for hiding this comment

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

maybe also call it allow_sentence_beginning_with_there_is? Not sure, as it's quite clear what's meant^^

Copy link
Member Author

Choose a reason for hiding this comment

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

I've followed the standard used in https://github.com/coala/coala-bears/blob/master/bears/natural_language/WriteGoodLintBear.py#L40

Maybe we should rename both the instances or keep this as it is ?

Copy link
Member

Choose a reason for hiding this comment

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

same, let's keep :)

Allows ``There is`` or ``There are`` at the beginning of a
sentence.
:param allow_ambiguous_words:
Allows ``weasel words`` for example ``often``, ``probably``.
Copy link
Member

@Makman2 Makman2 Jun 15, 2017

Choose a reason for hiding this comment

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

-->

Allows "weasel words" like "often" or "probably".

I think we should use regular double quotes as those are words referring to standard prose. For me code-highlighting means it's referring to some kind of specific technical/informatics term^^

Copy link
Member

Choose a reason for hiding this comment

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

Or

Allows "weasel words", for example "often" or "probably".

Copy link
Member Author

Choose a reason for hiding this comment

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

I've used the same documentation as here. So, is it okay or do we need to change it ?

Copy link
Member

Choose a reason for hiding this comment

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

Let's change, I think we should also change it for WriteGoodLintBear (there's also missing a period actually^^)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opening an issue in a while ;)

@yash-nisar
Copy link
Member Author

Updated @Makman2


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

@Makman2 Makman2 Jun 17, 2017

Choose a reason for hiding this comment

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

don't forget that we use raw-strings :D \u2713 will be rendered here as \u2713 actually and not :)

Copy link
Member

Choose a reason for hiding this comment

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

ah nvm, regex parses those \u escapes itself :D

class TextLintBear:
"""
The pluggable linting tool for text and Markdown. It is similar to ESLint,
but covers natural language (instead).
Copy link
Member

Choose a reason for hiding this comment

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

either
but covers natural language.
or
but covers natural language instead.
Sorry I think I should have been more clear with that :P

"textlint-rule-period-in-list-item": "~0.2.0",
"textlint-rule-rousseau": "~1.4.5",
"textlint-rule-unexpanded-acronym": "~1.2.1",
"textlint-rule-write-good": "~1.6.0",
Copy link
Member

Choose a reason for hiding this comment

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

just curious: Our package requirements don't support the ~ operator, right? (yet)
to indicate a rough version specification^^

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC, nope. Maybe that is why we specify the exact version like NpmRequirement('textlint', '7.3.0')

@Makman2
Copy link
Member

Makman2 commented Jun 17, 2017

ack d09c8f9

@Makman2
Copy link
Member

Makman2 commented Jun 17, 2017

@rultor merge

@rultor
Copy link

rultor commented Jun 17, 2017

@rultor merge

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

@rultor rultor merged commit d09c8f9 into coala:master Jun 17, 2017
@rultor
Copy link

rultor commented Jun 17, 2017

@rultor merge

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

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

Successfully merging this pull request may close these issues.

Bear proposal: TextlintBear
5 participants