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

HTMLLintBear: Add settings for individual features #1304

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
94 changes: 93 additions & 1 deletion bears/hypertext/HTMLLintBear.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,101 @@ class HTMLLintBear:

@staticmethod
def create_arguments(filename, file, config_file,
enable_doctype: bool=True,
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 check_doctype ? 😅

enable_entities: bool=True,
enable_trailing_whitespace: bool=True,
Copy link
Member

Choose a reason for hiding this comment

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

use allow_trailing_whitespace for consistency

enable_tabs: bool=True,
Copy link
Member

Choose a reason for hiding this comment

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

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

enable_charset: bool=True,
enable_void_element: bool=True,
enable_optional_tag: bool=True,
enable_type_attribute: bool=True,
enable_concerns_separation: bool=True,
enable_protocol: bool=True,
enable_names: bool=True,
enable_capitalization: bool=True,
enable_quotation: bool=True,
enable_indentation: bool=True,
enable_formatting: bool=True,
enable_boolean_attribute: bool=True,
enable_invalid_attribute: bool=True,
enable_void_zero: bool=True,
enable_invalid_handler: bool=True,
enable_http_equiv: bool=True,
enable_extra_whitespace: bool=True,
htmllint_ignore: typed_list(str)=()):
"""
:param htmllint_ignore: List of checkers to ignore.
:param enable_doctype:
Enable or disable doctype checker.
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 should add an example here, it is not quite clear what this setting does

:param enable_entities:
Enable or disable unicode entity checker.
Copy link
Member

Choose a reason for hiding this comment

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

here too...

:param enable_trailing_whitespace:
Enable or disable checker for trailing whitespace.
:param enable_tabs:
Copy link
Member

Choose a reason for hiding this comment

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

this should be use_spaces to be consistent with other bears. I've raised that as a separate issue #1741 , which you are welcome to mark as solved in this PR , but it is kind of urgent.

Enable or disable check for tabs.
:param enable_charset:
Enable or disable utf-8 checking.
:param enable_void_element:
Enable or disable checker for void tags.
:param enable_optional_tag:
Enable or disable checker for optional tags.
:param enable_type_attribute:
Enable or disable checker for default types.
:param enable_concerns_separation:
Enable or disable checker for seperation of concern.
:param enable_protocol:
Enable or disable checker for protocol specification.
:param enable_names:
Enable or disable checker for id or class name delimiters.
:param enable_capitalization:
Enable or disable checker for capitalization
of tags and attributes.
:param enable_quotation:
Enable or disable checker for quotation marks.
:param enable_indentation:
Enable or disable checker for indentation using spaces.
:param enable_formatting:
Enable or disable checker for general formatting.
:param enable_boolean_attribute:
Enable or disable checker for boolean attributes.
:param enable_invalid_attribute:
Enable or disable checker for invalid attributes.
:param enable_void_zero:
Enable or disable checker for javascript void(0).
:param enable_invalid_handler:
Enable or disable checker for Javascript links.
:param enable_http_equiv:
Enable or disable checker for http-equiv.
:param enable_extra_whitespace:
Enable or disable checker for extra whitespace.
:param htmllint_ignore:
List of checkers to ignore.
Copy link
Member

Choose a reason for hiding this comment

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

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

"""
param_dict = {
'doctype': enable_doctype,
'entities': enable_entities,
'trailing_whitespace': enable_trailing_whitespace,
'tabs': enable_tabs,
'charset': enable_charset,
'void_element': enable_void_element,
'optional_tag': enable_optional_tag,
'type_attribute': enable_type_attribute,
'concerns_separation': enable_concerns_separation,
'protocol': enable_protocol,
'names': enable_names,
'capitalization': enable_capitalization,
'quotation': enable_quotation,
'indentation': enable_indentation,
'formatting': enable_formatting,
'boolean_attribute': enable_boolean_attribute,
'invalid_attribute': enable_invalid_attribute,
'void_zero': enable_void_zero,
'invalid_handler': enable_invalid_handler,
'http_equiv': enable_http_equiv,
'extra_whitespace': enable_extra_whitespace,
}

ignore = ','.join(part.strip() for part in htmllint_ignore)
for param, use in param_dict.items():
if not use and param not in htmllint_ignore:
ignore += ',{}'.format(param)
return HTMLLintBear._html_lint, '--disable=' + ignore, filename
7 changes: 7 additions & 0 deletions tests/hypertext/HTMLLintBearTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,10 @@
invalid_files=(test_file,),
settings={'htmllint_ignore': 'quotation'},
tempfile_kwargs={'suffix': '.html'})

HTMLLintBearDisableCheckerTest = verify_local_bear(
HTMLLintBear,
valid_files=(test_file,),
invalid_files=(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is no invalid_file given?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The invalid_files are tested in a seperate test.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@Mixih Mixih Jan 16, 2017

Choose a reason for hiding this comment

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

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.

settings={'enable_optional_tag': 'false'},
tempfile_kwargs={'suffix': '.html'})