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

WIP: XMLBear: Support for style specifications #1187

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
35 changes: 29 additions & 6 deletions bears/xml2/XMLBear.py
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 (
Expand All @@ -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.
"""
Expand All @@ -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)
Copy link
Member

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

if style not in XMLBear._styles:
logging.warn('Unrecognised style ' + style + '. Valid xml'
' styles are c14n, c14n11, exc-c14n and oldxml10.'
Copy link
Member

Choose a reason for hiding this comment

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

you should rather do something like "Unrecognised style {}. Valid styles are {}. ......".format(...) we have a join_names function in coala (which should be moved to coala-utils: coala/coala#3486) that can concatenate them properly

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

This was merged, and is ready to use.

Copy link
Member

Choose a reason for hiding this comment

The 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'}
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sils can i just copy the information in (xmllint --help) like
--c14n : save in W3C canonical format v1.0 (with comments)

"""
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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was given in the issue by @jayvdb, you can check it out here #1098


return args

Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

Isnt this a thread unsafe operation?
Is that a problem? cc: @Makman2

Copy link
Member

Choose a reason for hiding this comment

The 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 self._diff_severity with just diff_severity 👍

result_message='XML can be formatted better.'))
else:
# Return issues from stderr if stdout is empty
Expand Down
31 changes: 31 additions & 0 deletions tests/xml2/XMLBearTest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -36,6 +37,10 @@ def load_testdata(filename):
<a>hey and hi</a>
"""

invalid_xml_file_c14n = """<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

The 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')
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

can we have a valid file too?