-
Notifications
You must be signed in to change notification settings - Fork 581
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
WIP: XMLBear: Support for style specifications #1187
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import itertools | ||
import re | ||
import logging | ||
|
||
from coalib.bearlib.abstractions.Linter import linter | ||
from dependency_management.requirements.DistributionRequirement import ( | ||
|
@@ -11,7 +12,6 @@ | |
def path_or_url(xml_dtd): | ||
""" | ||
Converts the setting value to url or path. | ||
|
||
:param xml_dtd: Setting key. | ||
:return: Returns a converted setting value. | ||
""" | ||
|
@@ -21,13 +21,27 @@ def path_or_url(xml_dtd): | |
return path(xml_dtd) | ||
|
||
|
||
def xml_style_valid(xml_style): | ||
""" | ||
Checks if xml_style is valid. | ||
:param xml_style: Setting key. | ||
:return: Returns the value if valid else returns none. | ||
""" | ||
style = str(xml_style) | ||
if style not in XMLBear._styles: | ||
logging.warn('Unrecognised style ' + style + '. Valid xml' | ||
' styles are c14n, c14n11, exc-c14n and oldxml10.' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should rather do something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ideally coala/coala#3486 should be solved first, that'll save us some hassle later There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was merged, and is ready to use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not an error, or even exception? Do we habe guidance on this? |
||
' Running XMLBear without any xml_style argument.') | ||
return None | ||
return '--' + style | ||
|
||
|
||
@linter(executable='xmllint', | ||
use_stdout=True, | ||
use_stderr=True) | ||
class XMLBear: | ||
""" | ||
Checks the code with ``xmllint``. | ||
|
||
See http://xmlsoft.org/xmllint.html | ||
""" | ||
LANGUAGES = {'XML'} | ||
|
@@ -40,21 +54,30 @@ class XMLBear: | |
_output_regex = re.compile( | ||
r'.*:(?P<line>\d+):.*(?P<severity>error|warning)\s?: ' | ||
r'(?P<message>.*)\n.*\n.*') | ||
_diff_severity = RESULT_SEVERITY.INFO | ||
_styles = ('c14n', 'c14n11', 'exc-c14n', 'oldxml10') | ||
|
||
@staticmethod | ||
def create_arguments(filename, file, config_file, | ||
def create_arguments(self, filename, file, config_file, | ||
xml_schema: path='', | ||
xml_dtd: path_or_url=''): | ||
xml_dtd: path_or_url='', | ||
xml_style: xml_style_valid=None): | ||
""" | ||
:param xml_schema: ``W3C XML Schema`` file used for validation. | ||
:param xml_dtd: ``Document type Definition (DTD)`` file or | ||
url used for validation. | ||
:param xml_style: ``XML Style Specification`` Relevant args are | ||
c14n, c14n11, exc-c14n and oldxml10. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you include a URL where I can find out more about those cryptic things? Can we maybe ake them less cryptic? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you need to include that info here so the user can find out more about the specs, not me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, ok !! I misunderstood you earlier |
||
Find out more about the formats at | ||
https://www.w3.org/TR/#tr_XML_Canonicalization | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you rather give a link where I can immediately see what those things mean for my code? This is not helpful (maybe a bit) but almost nobody will actually browse that website since it doesn't seem to contain the desired info easily available. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sils can i just copy the information in (xmllint --help) like |
||
""" | ||
args = (filename,) | ||
if xml_schema: | ||
args += ('-schema', xml_schema) | ||
if xml_dtd: | ||
args += ('-dtdvalid', xml_dtd) | ||
if xml_style: | ||
args += (xml_style,) | ||
self._diff_severity = RESULT_SEVERITY.MAJOR | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this major? Also if you do this, all the other errors in the diff will have a major severity as well, right? That sounds bad. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
return args | ||
|
||
|
@@ -68,7 +91,7 @@ def process_output(self, output, filename, file): | |
output_regex=self._output_regex), | ||
self.process_output_corrected( | ||
stdout, filename, file, | ||
diff_severity=RESULT_SEVERITY.INFO, | ||
diff_severity=self._diff_severity, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isnt this a thread unsafe operation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in praxis it's not (due to python executing everything in several processes), though this is a local variable and shall be used as such. So replace |
||
result_message='XML can be formatted better.')) | ||
else: | ||
# Return issues from stderr if stdout is empty | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
from coalib.results.RESULT_SEVERITY import RESULT_SEVERITY | ||
from coalib.settings.Section import Section | ||
from coala_utils.ContextManagers import prepare_file | ||
from coalib.settings.Setting import Setting | ||
|
||
|
||
def load_testdata(filename): | ||
|
@@ -36,6 +37,10 @@ def load_testdata(filename): | |
<a>hey and hi</a> | ||
""" | ||
|
||
invalid_xml_file_c14n = """<?xml version="1.0"?> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear without knowing the c14n style, why this is invalid. Could you please add a comment explaining why this fails under this style? |
||
<a/> | ||
""" | ||
|
||
dtd_file = os.path.join(os.path.dirname(__file__), | ||
'test_files', | ||
'note.dtd') | ||
|
@@ -98,3 +103,29 @@ def test_errors(self): | |
with prepare_file(content, None) as (file, fname): | ||
with execute_bear(self.uut, fname, file) as results: | ||
self.assertEqual(results[0].severity, RESULT_SEVERITY.MAJOR) | ||
|
||
|
||
@generate_skip_decorator(XMLBear) | ||
class XMLBearStyleTest(unittest.TestCase): | ||
|
||
def setUp(self): | ||
self.section = Section('name') | ||
self.uut = XMLBear(self.section, Queue()) | ||
|
||
def test_xml_style_errors(self): | ||
self.section.append(Setting('xml_style', 'c14n')) | ||
content = invalid_xml_file_c14n.splitlines() | ||
with prepare_file(content, None) as (file, fname): | ||
with execute_bear(self.uut, fname, file) as results: | ||
self.assertEqual(results[0].message, | ||
'XML can be formatted better.') | ||
self.assertEqual(results[0].severity, RESULT_SEVERITY.MAJOR) | ||
|
||
def test_unrecognised_xml_style_name(self): | ||
self.section.append(Setting('xml_style', 'wrong-args')) | ||
content = invalid_xml_file.splitlines() | ||
with prepare_file(content, None) as (file, fname): | ||
with execute_bear(self.uut, fname, file) as results: | ||
self.assertEqual(results[0].message, | ||
'XML can be formatted better.') | ||
self.assertEqual(results[0].severity, RESULT_SEVERITY.INFO) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we have a valid file too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you delete this, unrelated and I don't think that should happen anyway