[#4044] Add semantic newline checker in lint #7
Conversation
|
needs-review |
hcs0
changed the title
|
look good. thanks! Please make sure that for python you are using spaces instead of tabs. Your code was a mix of tags and spaces, and this can lead to strange bugs. just a few minor design related comments. I have pushed a few cleanups....and will look at fixing the tests. I will look at enabling travis-ci tests for this project so that we can get a feedback faster. You can test the change on chevah/server using Before merging this we need to clean up the scame project so that all tests are green |
scame/tests/test_restructuredtext.py
Outdated
| @@ -44,6 +44,13 @@ | |||
| | verse, and adornment-free lists. | |||
|
|
|||
|
|
|||
| Newline test! No newline. | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
scame/tests/test_restructuredtext.py
Outdated
| self.assertEqual(expect, self.reporter.messages) | ||
| self.assertEqual(1, self.reporter.call_count) | ||
|
|
||
| def test_semantic_newline_exclamationmark_true(self): |
There was a problem hiding this comment.
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'
)
scame/tests/test_restructuredtext.py
Outdated
| 'Sentence. New sentence\n' | ||
| ) | ||
| checker = ReStructuredTextChecker('bogus', content, self.reporter) | ||
| checker.check_semantic_newline() |
There was a problem hiding this comment.
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
scame/__version__.py
Outdated
| @@ -1,4 +1,4 @@ | |||
| """ | |||
| Keeps the version of the project. | |||
| """ | |||
| VERSION = '0.3.1' | |||
| VERSION = '0.3.4' | |||
There was a problem hiding this comment.
I think that this should be 0.4.0 as is add a new feature... 0.3.X is for bugfixes
|
I've built the package and installing it Please try on your branch. needs-review |
scame/formatcheck.py
Outdated
| @@ -461,6 +461,13 @@ def check(self): | |||
| class AnyTextMixin: | |||
| """Common checks for many checkers.""" | |||
|
|
|||
| def check_semantic_newline(self): | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
please check latest commits.
I have fixed all the other tests, so the remaining tests which are failing are the one for this branch.
I don't really understand what your code and your test is trying to do :(
I have replaced it with a basic implementation, which works for one test (test_check_semantic_newline_fullstop)
you can target a single test using
build/bin/nosetests scame.tests.test_restructuredtext:TestReStructuredTextChecker.test_check_semantic_newline_questionmark
scame/formatcheck.py
Outdated
| def check_semantic_newline(self): | ||
| """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', '..']): |
There was a problem hiding this comment.
I have no idea how and why this code works... but I guess that it does not work
first, who is self.line, second how is string.find() used, third what is at right-hand side (why the list, why the parenthesis)
scame/formatcheck.py
Outdated
| 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') |
scame/tests/test_restructuredtext.py
Outdated
| 'Sentence? \n' | ||
| ) | ||
| checker = ReStructuredTextChecker('bogus', content, self.reporter) | ||
| checker._check_semantic_newline() |
scame/tests/test_restructuredtext.py
Outdated
| """When a ., ?, or ! is not the last character of the line, it is | ||
| considered a semantic newline violation and an error is reported.""" | ||
| content = ( | ||
| 'Sentence. \n' |
There was a problem hiding this comment.
this test is wrong...as it will trigger the "trailing newspaces" checker
scame/tests/test_restructuredtext.py
Outdated
| ) | ||
| checker = ReStructuredTextChecker('bogus', content, self.reporter) | ||
| checker._check_semantic_newline() | ||
| expect = ['Check that a new sentence is created after a full stop, ! or ?.'] |
There was a problem hiding this comment.
the expected value is wrong. it should repot a tuple of line number and error message
Scope
This branch adds a semantic newline checker in scame
Changes
Add the rule in formatcheck.py
Add tests in test_restructuredtext.py
Updates from the last feedback provided on #2
How to try and test the changes
reviewers: @adiroiban