HTMLLintBear: Add settings for individual features #1304

pratyushprakash · 2017-01-14T09:40:38Z

Settings have been added for enabling/disabling indidual
features of the html_linter.

gitmate-bot · 2017-01-14T09:40:51Z

Thanks for your contribution!

Reviewing pull requests take really a lot of time and we're all volunteers. Please make sure you go through the following check list and complete them all before pinging someone for a review.

I have rebased properly. Please see this for a tutorial.
I have gone through the commit guidelines and I've followed them:
I have followed the guidelines for the commit shortlog.
I have followed the guidelines for the commit body.
I have included the issue URL.
I have used the Fixes keyword if the commit fixes a real bug and the Closes keyword if it adds a feature/enhancement. See more here.
I have run all the tests and they all pass. That includes that all CI (below) is green - if GitMate issues appear you will have to amend your commits, pushing new commits on top is not sufficient!

As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well!

gitmate-bot · 2017-01-14T09:41:29Z

tests/hypertext/HTMLLintBearTest.py

+    invalid_files=(),
+    settings={'optional_tag': 'off'},
+    tempfile_kwargs={'suffix': '.html'})
+


W391 blank line at end of file'

PycodestyleBear (W391), severity NORMAL, section autopep8.

gitmate-bot · 2017-01-14T09:41:29Z

tests/hypertext/HTMLLintBearTest.py

+    invalid_files=(),
+    settings={'optional_tag': 'off'},
+    tempfile_kwargs={'suffix': '.html'})
+


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/tests/hypertext/HTMLLintBearTest.py +++ b/tests/hypertext/HTMLLintBearTest.py @@ -34,4 +34,3 @@ invalid_files=(), settings={'optional_tag': 'off'}, tempfile_kwargs={'suffix': '.html'}) -

namanyadav12 · 2017-01-14T10:48:27Z

tests/hypertext/HTMLLintBearTest.py

+HTMLLintBearDisableCheckerTest = verify_local_bear(
+    HTMLLintBear,
+    valid_files=(test_file,),
+    invalid_files=(),


Why is no invalid_file given?

The invalid_files are tested in a seperate test.

For all tests we should have at least an valid and an invalid file

The tests before don't follow that. The invalid_files is already covered in the HTMLLintBearTest and HTMLLintBearIgnoreQuotationTest. I feel since HTMLLintBearDisableCheckerTest is supposed to check whether disabling a certain setting takes effect, it would make no sense to include an invalid_file.

@pratyushprakash How does this test check whether disabling a certain setting takes effect.

@namanyadav12 could you please view HTMLLintBearTest.py in its entirety and not just the changes I made

Imo testing on this bear is a bit screwy and needs to be improved. This would be separate issue. Ideally we test most of these options, but not possible right now. However since we are trying to improve tests, it would be nice if you verified that the bear was outputting the correct error.

pratyushprakash · 2017-01-14T14:09:34Z

@namanyadav12 What exactly do you want changed ?

aptrishu · 2017-01-15T14:52:13Z

tests/hypertext/HTMLLintBearTest.py

+    HTMLLintBear,
+    valid_files=(test_file,),
+    invalid_files=(),
+    settings={'optional_tag': 'off'},


can you add a test with an invalid tag ?
html-lint handles it so we need just to check we are good with that.

Well apparently html-lint does not check for invalid tags.

I think it does see https://github.com/sk-/html-linter/blob/master/html_linter.py#L1083 ..

That peice of code is actually informing the user that they provided invalid disable arguments.

yeah, sorry by tag earlier I was referring to disable argument. damn. >_<

AsnelChristian · 2017-01-15T15:00:43Z

bears/hypertext/HTMLLintBear.py

@@ -28,9 +28,95 @@ class HTMLLintBear:

    @staticmethod
    def create_arguments(filename, file, config_file,
+                         doctype: str='Enable',
+                         entities: str='Enable',
+                         trailing_whitespace: str='Enable',


most of the settings start with either allow or enable or force... you should consider using that convention, and particularly for this setting we use allow_trailing_whitespaces [use git grep to check ]

And the settings are of type bool if they are true/false type flags.

Settings have been added for enabling/disabling indidual features of the `html_linter`. Closes coala#1247

AsnelChristian · 2017-01-16T07:41:38Z

bears/hypertext/HTMLLintBear.py

@@ -28,9 +28,101 @@ class HTMLLintBear:

    @staticmethod
    def create_arguments(filename, file, config_file,
+                         enable_doctype: bool=True,
+                         enable_entities: bool=True,
+                         enable_trailing_whitespace: bool=True,


use allow_trailing_whitespace for consistency

AsnelChristian · 2017-01-16T07:42:47Z

bears/hypertext/HTMLLintBear.py

+                         enable_doctype: bool=True,
+                         enable_entities: bool=True,
+                         enable_trailing_whitespace: bool=True,
+                         enable_tabs: bool=True,


either use_tabs: bool=True or use_spaces: bool=False

AsnelChristian · 2017-01-16T07:44:19Z

bears/hypertext/HTMLLintBear.py

@@ -28,9 +28,101 @@ class HTMLLintBear:

    @staticmethod
    def create_arguments(filename, file, config_file,
+                         enable_doctype: bool=True,


what about check_doctype ? 😅

AsnelChristian · 2017-01-16T07:45:29Z

bears/hypertext/HTMLLintBear.py

                         htmllint_ignore: typed_list(str)=()):
        """
-        :param htmllint_ignore: List of checkers to ignore.
+        :param enable_doctype:
+            Enable or disable doctype checker.


maybe you should add an example here, it is not quite clear what this setting does

AsnelChristian · 2017-01-16T07:45:41Z

bears/hypertext/HTMLLintBear.py

+        :param enable_doctype:
+            Enable or disable doctype checker.
+        :param enable_entities:
+            Enable or disable unicode entity checker.


here too...

AsnelChristian · 2017-01-16T07:47:28Z

bears/hypertext/HTMLLintBear.py

+        :param enable_extra_whitespace:
+           Enable or disable checker for extra whitespace.
+        :param htmllint_ignore:
+           List of checkers to ignore.


i feel like, you should instead tell what the setting do instead of telling what is enabled or disabled

gitmate-bot · 2017-01-23T15:45:50Z