[#4044] Add semantic newline checker in lint (happy hacking) #2

hcs0 · 2017-05-06T00:24:06Z

Scope

This branch adds a semantic newline checker in scame.

Changes

Add the rule in formatcheck.py
Add tests in test_restructuredtext.py

How to try and test the changes

I followed the rules to do a manual test by running sdist and then linking to the new paver dist.

I am having trouble validating the code when I run paver lint --all in that I can't see what the proper output is.
See #5

I am also trying to follow the format with existing test like def test_multiple_empty_last_lines(self)

Initial try. Would be good to know how to run a use test case against this.

hcs0 · 2017-05-06T00:25:34Z

@adiroiban How to check if this works?

adiroiban · 2017-05-06T08:55:05Z

This needs to be branched from and merged into the chevah-master branch -
https://github.com/chevah/pocket-lint/tree/chevah-master

To check that it works you first write automated tests . See https://github.com/chevah/pocket-lint/blob/chevah-master/pocketlint/tests/test_restructuredtext.py

For manual tests, you can update setup.py to let's say version="1.4.4.c13", ... and then run python setup.py sdist ... this will create the python source package in the 'dist/' folder.

Now... once you have the package you can manually install it into the server repo

cd path/to/server/repo/base
./build-OS-CPU/bin/python -m pip install -U ../path/to/pocket-lint/dist/pocket-list-package.1.2.3.tar.gz`
paver lint --all

Note that paver lint by default will only check the files changed since master... so you need the --all flag to recheck even the files which were not change.

Let me know if you need more help

Thanks!

adiroiban · 2017-05-06T08:56:44Z

pocketlint/formatcheck.py

@@ -939,6 +945,7 @@ def check_lines(self):
        for line_no, line in enumerate(self.lines):
            line_no += 1
            self.check_length(line_no, line)
+            self.check_semantic_newline()


this is fine, but don't forget to pass the line_no, line arguments to this check.

line_no is important so that the error report will pin-point the place with the error

adiroiban · 2017-05-06T08:59:25Z

pocketlint/formatcheck.py

@@ -460,6 +460,12 @@ def check_regex_line(self, line_no, line):
                    icon='info',
                    )

+def check_semantic_newline(self):
+        """Check for a sentence with no new line."""
+        if self.text.find('. ') and self.text[-1] != '\n':


self.text is the whole file... as commented below you should pass the line_no and line here and do the search only inside that line.

searching only for line.find('. ') should be enough... no need for line[-1]

we might also want to search for question mark and exclamation mark.

once this is done, do a full check on our full documentation to see for any false positives

adiroiban · 2017-05-06T13:44:29Z

@hcs0 you should now have write access to this repo... some time ago, GitHub has updated the permissions model... and we have to manually assign the permissions on all our existing repos

this was left without an update... and there might be more. you should now have admin rights to this repo

hcs0 · 2017-05-08T15:50:07Z

Thanks, I will pick this up again next happy hacking day

- Check for ? and ! - Ignore double stops

hcs0 · 2017-07-06T11:09:40Z

@adiroiban Please take a quick look to see if what I have is OK.
I'm working on writing a test for this and testing it.

adiroiban · 2017-07-06T12:05:23Z

pocketlint/formatcheck.py

@@ -460,6 +460,12 @@ def check_regex_line(self, line_no, line):
                    icon='info',
                    )

+def check_semantic_newline(self):


bad indentation

adiroiban · 2017-07-06T12:11:47Z

pocketlint/formatcheck.py

+def check_semantic_newline(self):
+        """Check for a sentence with no new line."""
+        if self.line.find(['. ', '? ', '! ']) != (['\n', '..']):
+            self.message(


I don't understand what logic is used here.
It helps to add a comment describing what are you targeting here.

looking forward for the tests, as I hope that they can provide more documentation

This checks for
.
?
!

and excludes new lines and .. (for the adnotations)

not sure if I did it correctly though so I think tests would be useful

Previously it was only checking for full stop and space .

hcs0 · 2017-07-07T11:17:31Z

@adiroiban I added a few tests and created a new paver distro to check lint. But having issues running paver lint --all. Logs don't point to lines that I've added in. I've created a new issue at #5

Now for the code, I'd rather test this using paver --all to see if the actual change itself is working (ie learning via trial/error)

hcs0 · 2017-07-07T22:39:40Z

needs-review

adiroiban

Looks good.

Please extend the test to also cover valid use cases, not only the negative tests.

You should have write access to the repo, so feel free to create a branch directly into this repo.. and not into a fork.

also, please merge into master branch and not trunk.
Since moving to 'scamewe now usemaster` as te main branch.

great work!

adiroiban · 2017-07-10T07:59:38Z

pocketlint/formatcheck.py

@@ -460,6 +460,14 @@ def check_regex_line(self, line_no, line):
                    icon='info',
                    )

+    def check_semantic_newline(self):
+        """Check for a sentence with a ., ?, or ! with a space.


I think that for the new code we can slowly migrate to our latest styleguide.

for this docstring, i want to read a human readable version of what rules are used to check for semantic newlines

adiroiban · 2017-07-10T08:01:53Z

pocketlint/tests/test_restructuredtext.py

@@ -343,6 +347,39 @@ def test_check_section_delimiter_bad_marker_length(self):
        self.assertEqual(expect, self.reporter.messages)
        self.assertEqual(1, self.reporter.call_count)

+    def test_semantic_newline_fullstop(self):
+        """Check for a sentence with a . and no new line."""


this docstring does not help to much.

see http://styleguide.chevah.com/testing.html#tests-description-docstrings

It should sound something like.

"""
When a full stop is not the last character from the line, it is considered a semantic newline violation and an error is reported.
"""

adiroiban · 2017-07-10T08:02:31Z

pocketlint/tests/test_restructuredtext.py

+    def test_semantic_newline_fullstop(self):
+        """Check for a sentence with a . and no new line."""
+        content = (
+            'Sentence. New sentence\n'


for this input, it would help to have some valid semantic newlines just to make sure that we don't get false positives

adiroiban · 2017-07-10T08:04:30Z

pocketlint/tests/test_restructuredtext.py

+            )
+        checker = ReStructuredTextChecker('bogus', content, self.reporter)
+        checker.check_semantic_newline()
+        expect = [('Newline not created after a sentence.')]


what is the point of these parenthesis here?

I know that existing tests are not a good example, but for new tests please consider using the structure recomended by the styleguide http://styleguide.chevah.com/testing.html#structure-of-a-test

adiroiban · 2017-07-10T08:05:43Z

setup.py

@@ -52,7 +52,7 @@ def run(self):
 setup(
    name="pocketlint",
    description="Pocket-lint a composite linter and style checker.",
-    version="1.4.4",
+    version="1.4.4.c13",


why add .c13 here.

there is a bug here as name should be scame and not pocketlint

you might be working on an old fork

I added .c13 from your comment :)
#2 (comment)

I'll look into moving this into a newer fork / and merging into the correct branch next HH day

hcs0 · 2017-07-10T08:39:41Z

@adiroiban Should there also be codecov in scame repo?

adiroiban · 2017-07-10T09:08:37Z

@adiroiban Should there also be codecov in scame repo?

yes.... but to have codecov we need to enable travis-ci ... and to enable travis-ci we first need to fix all our tests :)

hcs0 · 2017-08-23T12:20:11Z

Closing this since the new branch will merge to the master trunk

hcs0 and others added 2 commits May 6, 2017 02:11

[#4044] Add semantic newline checker

3c5cfe5

Initial try. Would be good to know how to run a use test case against this.

Rewrite comment

e34c3ba

adiroiban reviewed May 6, 2017

View reviewed changes

After review

5758ff7

- Check for ? and ! - Ignore double stops

adiroiban reviewed Jul 6, 2017

View reviewed changes

Hannah Suarez hcs0 added 2 commits July 6, 2017 14:15

Fix indentation and improve comment

8e2cf82

Add tests and change setup.py dist version to test paver

cb4da3f

hcs0 mentioned this pull request Jul 7, 2017

Fix paver lint --all to test new pocket-lint/scame #5

Open

Minor updates

af562d5

hcs0 changed the title ~~[#4044] Add semantic newline checker in lint~~ [#4044] Add semantic newline checker in lint (happy hacking) Jul 10, 2017

adiroiban requested changes Jul 10, 2017

View reviewed changes

hcs0 closed this Aug 23, 2017

This was referenced Aug 23, 2017

[#4044] Add semantic newline checker in lint #7

Open

[#4264] field_name_limit should not expand to 2 columns chevah/docutils#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#4044] Add semantic newline checker in lint (happy hacking) #2

[#4044] Add semantic newline checker in lint (happy hacking) #2

hcs0 commented May 6, 2017 •

edited

Loading

hcs0 commented May 6, 2017

adiroiban commented May 6, 2017

adiroiban May 6, 2017

adiroiban May 6, 2017

adiroiban commented May 6, 2017

hcs0 commented May 8, 2017

hcs0 commented Jul 6, 2017

adiroiban Jul 6, 2017

adiroiban Jul 6, 2017

hcs0 Jul 6, 2017

hcs0 commented Jul 7, 2017

hcs0 commented Jul 7, 2017

adiroiban left a comment

adiroiban Jul 10, 2017

adiroiban Jul 10, 2017

adiroiban Jul 10, 2017

adiroiban Jul 10, 2017

adiroiban Jul 10, 2017

hcs0 Jul 10, 2017 •

edited

Loading

hcs0 commented Jul 10, 2017

adiroiban commented Jul 10, 2017

hcs0 commented Aug 23, 2017

[#4044] Add semantic newline checker in lint (happy hacking) #2

[#4044] Add semantic newline checker in lint (happy hacking) #2

Conversation

hcs0 commented May 6, 2017 • edited Loading

Scope

Changes

How to try and test the changes

hcs0 commented May 6, 2017

adiroiban commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban commented May 6, 2017

hcs0 commented May 8, 2017

hcs0 commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcs0 commented Jul 7, 2017

hcs0 commented Jul 7, 2017

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcs0 Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

hcs0 commented Jul 10, 2017

adiroiban commented Jul 10, 2017

hcs0 commented Aug 23, 2017

hcs0 commented May 6, 2017 •

edited

Loading

hcs0 Jul 10, 2017 •

edited

Loading