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.

Sign up for GitHub

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

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

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
797ac80
Initial commit with new tests based on feedback
Aug 23, 2017
24c7424
Fix docstrings
Aug 23, 2017
fd6e712
Some cleanups.
adiroiban Aug 31, 2017
133044a
Add version details
Sep 7, 2017
a903fe4
Add test to encompass all
Sep 7, 2017
031b4ad
Update release versions
Sep 7, 2017
75252ec
Refactor regex semantic line checker.
adiroiban Sep 12, 2017
cd9554a
Fix own code to pass newline rst checker.
adiroiban Sep 12, 2017
4a3f949
Fix other tests.
adiroiban Sep 12, 2017
3fad50a
Merge branch 'master' into 4044-semantic-newline-checker
Nov 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions scame/formatcheck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,13 @@ def check(self):
class AnyTextMixin:
"""Common checks for many checkers."""

def check_semantic_newline(self):
Copy link
Member

Choose a reason for hiding this comment

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

this is defined here, but is not 'enabled' in any checkers.

also, since for now we plan to use it only for RST it should be moved into the RST checker.

"""Check that there are ., ?, or ! with a space following after.
Exclude those with no new line and two full-stops."""
if self.line.find(['. ', '? ', '! ']) != (['\n', '..']):
self.message(
0, 'Line contains a sentence without a new line.', icon='info')

def check_conflicts(self, line_no, line):
"""Check that there are no merge conflict markers."""
if line.startswith('<' * 7) or line.startswith('>' * 7):
Expand Down Expand Up @@ -1035,7 +1042,7 @@ def check_ascii(self, line_no, line):
line.encode('ascii')
except UnicodeEncodeError as error:
self.message(
line_no, 'Non-ascii characer at position %s.' % error.end,
line_no, 'Non-ascii character at position %s.' % error.end,
icon='error',
)

Expand Down Expand Up @@ -1243,14 +1250,14 @@ def isSectionDelimiter(self, line_number):
def check_section_delimiter(self, line_number):
"""Checks for section delimiter.

These checkes are designed for sections delimited by top and bottom
These checks are designed for sections delimited by top and bottom
markers.

======= <- top marker
Section <- text_line
======= <- bottom marker

If the section is delimted only by bottom marker, the section text
If the section is delimited only by bottom marker, the section text
is considered the top marker.

Section <- top marker, text_line
Expand Down
85 changes: 82 additions & 3 deletions scame/tests/test_restructuredtext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@


--------------------
Second emtpy section
Second empty section
--------------------


Third section
^^^^^^^^^^^^^

Paragrhap for
Paragraph for
third section `with link<http://my.home>`_.

::
Expand All @@ -44,6 +44,13 @@
| verse, and adornment-free lists.


Newline test! No newline.
Copy link
Member

Choose a reason for hiding this comment

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

this data is called valid_rst_content but from what I can see here, you are adding invalid elements to it.

but, you have introduced invalid content here, yet no test using this data was updated. Something is not right here.

check_semantic_newline is not actually used.

I think that it should be defined in the RST checker and enabled there

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's not really clear to me what this section does.
I assume that this is where you add some tests. However test_semantic_newline_fullstop_true(self): contains some tests.
I don't need to add to this section

Copy link
Member

Choose a reason for hiding this comment

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

That data is used in test_valid_content. As described in the comment, it is not placed closed to test_valid_content, but rather at the top to make it easier to debug the test

Is ok to update the content of this data. but it should be updated only with valid data.

this is a regression test to check that we don't have false-positives

Newline test? No newline.
Newline test. No newline.
Newline test has newline.
Indeed.


.. _section-permalink:

Another Section with predefined link
Expand Down Expand Up @@ -141,7 +148,7 @@ def test_no_empty_last_line(self):
self.reporter.call_count = 0
content = (
'Some first line\n'
'the second and last line witout newline')
'the second and last line without newline')
checker = ReStructuredTextChecker('bogus', content, self.reporter)
checker.check_empty_last_line(2)
expected = [(
Expand Down Expand Up @@ -343,6 +350,78 @@ 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):
"""When a full stop is not the last character of the line, it is
considered a semantic newline violation and an error is reported."""
content = (
'Sentence. New sentence\n'
)
checker = ReStructuredTextChecker('bogus', content, self.reporter)
checker.check_semantic_newline()
Copy link
Member

Choose a reason for hiding this comment

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

the code from scame is old and it has a lot of bad habits.
see the comment for valid_rst_content.

The design should be that check_semantic_newline to be renamed _check_semantic_newline and made private member.

The tests should use instead the base check method or only the methods from BaseChecker

expect = ['Newline not created after a sentence.']
self.assertEqual(expect, self.reporter.messages)
self.assertEqual(1, self.reporter.call_count)

def test_semantic_newline_fullstop_true(self):
"""When a full stop is the last character from the line and follows the
semantic newline rule, an error is not reported."""
content = (
'Sentence. \n'
'New sentence.'
)
checker = ReStructuredTextChecker('bogus', content, self.reporter)
checker.check_semantic_newline()
self.assertEqual([], self.reporter.messages)
self.assertEqual(0, self.reporter.call_count)

def test_semantic_newline_questionmark(self):
"""When a question mark is not the last character of the line, it is
considered a semantic newline violation and an error is reported."""
content = (
'Sentence? New sentence\n'
)
checker = ReStructuredTextChecker('bogus', content, self.reporter)
checker.check_semantic_newline()
expect = [('Newline not created after a ? sentence.')]
self.assertEqual(expect, self.reporter.messages)
self.assertEqual(1, self.reporter.call_count)

def test_semantic_newline_questionmark_true(self):
"""When a question mark is the last character from the line and follows
semantic newline rule, an error is not reported."""
content = (
'Sentence? \n'
'New sentence\n'
)
checker = ReStructuredTextChecker('bogus', content, self.reporter)
checker.check_semantic_newline()
self.assertEqual([], self.reporter.messages)
self.assertEqual(0, self.reporter.call_count)

def test_semantic_newline_exclamationmark(self):
"""When an exclamation mark is not the last character of the line, it
is considered a semantic newline violation and an error is reported."""
content = (
'Sentence! New sentence\n'
)
checker = ReStructuredTextChecker('bogus', content, self.reporter)
checker.check_semantic_newline()
expect = [('Newline not created after a ! sentence.')]
self.assertEqual(expect, self.reporter.messages)
self.assertEqual(1, self.reporter.call_count)

def test_semantic_newline_exclamationmark_true(self):
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what to say about all these tests. I feel like there is a lot of duplication in these tests.

So maybe just have a single test in which the content is

content = (
    'Sentence! \n'
    'Sentence. \n'
    'Sentence? \n'
    )

"""When an exclamation mark is the last character from the line and
follows the semantic newline rule, an error is not reported."""
content = (
'Sentence! \n'
'New sentence\n'
)
checker = ReStructuredTextChecker('bogus', content, self.reporter)
checker.check_semantic_newline()
self.assertEqual([], self.reporter.messages)
self.assertEqual(0, self.reporter.call_count)

def test_check_section_delimiter_bad_length_both_markers(self):
content = (
'---------\n'
Expand Down