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

Rewrite flycheck-parse-xml-region (use xml-parse-region if libxml fails) #1349

Merged
merged 1 commit into from Nov 7, 2017

Conversation

@cpitclaudel
Copy link
Member

commented Oct 25, 2017

Closes GH-1298, GH-1330.

@fmdkdd

fmdkdd approved these changes Oct 26, 2017

Copy link
Member

left a comment

LGTM.

Although, I wonder if we shouldn't make a new function instead of modifying flycheck-xml-parse-region. Currently, for all the people who have set flycheck-xml-parser to flycheck-parse-xml-region, we might be running libxml first. This may be surprising (but will silently fail).

That being said, if it helps unearth errors in non-XML compliant linters, all the better.

flycheck.el Outdated
from the node list."
(car (xml-parse-region beg end)))
of the node list."
;; FIXME use `libxml-available-p' when it gets implemented.

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 26, 2017

Member

This won't be before, at least, Emacs 26, right?

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 26, 2017

Author Member

Correct

flycheck.el Outdated
;; FIXME use `libxml-available-p' when it gets implemented.
(or (and (fboundp 'libxml-parse-xml-region)
(libxml-parse-xml-region beg end))
(ignore-errors (car (xml-parse-region beg end)))))

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 26, 2017

Member

Is this wise to silence errors here?

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 26, 2017

Author Member

Do you mean here vs somewhere else, or silencing vs not silencing? If the latter, I don't think we have much of a choice: that's how libxml behaves (it just returns nil when it fails to parse), which we then complain about (unexpected output …). Otherwise we'd throw an error, and we don't currently do that.

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 26, 2017

Member

Okay, so now at least it's coherent with libxml.

@cpitclaudel

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2017

Although, I wonder if we shouldn't make a new function instead of modifying flycheck-xml-parse-region.

Indeed, absolutely.

@cpitclaudel cpitclaudel force-pushed the 1330_libxml branch from 92f5654 to 269b45f Oct 26, 2017

@fmdkdd

fmdkdd approved these changes Oct 26, 2017

Copy link
Member

left a comment

Looks good. This probably warrants a line in CHANGES, since it's a user-facing variable.

@cpitclaudel cpitclaudel force-pushed the 1330_libxml branch from 269b45f to ee7cb6d Nov 7, 2017

@cpitclaudel cpitclaudel merged commit ee7cb6d into master Nov 7, 2017

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@cpitclaudel cpitclaudel deleted the 1330_libxml branch Nov 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.