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

Add an error-explainer for rpm-rpmlint #1235

Merged
merged 1 commit into from Apr 11, 2017

Conversation

Projects
None yet
3 participants
@omajid
Contributor

omajid commented Mar 28, 2017

rpmlint's warnings/errors are very terse by default. For example, if the file contains the following:

Source0: foobar.tar.xz

The warning shown by flycheck is:

invalid-url Source0: foobar.tar.xz

The explainer helps clarify the root cause:

invalid-url:
The value should be a valid, public HTTP, HTTPS, or FTP URL.
@CLAassistant

This comment has been minimized.

CLAassistant commented Mar 28, 2017

CLA assistant check
All committers have signed the CLA.

@omajid omajid force-pushed the omajid:rpm-lint-error-explainer branch 2 times, most recently from 9da1480 to cbda8e1 Mar 28, 2017

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Mar 28, 2017

Ah, that's great! One more explainer 👍

Instead of extracting the error-code in :error-explainer, we should extract it in :error-patterns using (id). That way, you would just have to use flycheck-error-id to get it out, and the id is in a separate column in the error list.

On a related note, I tried to build rpmlint on an Archlinux VM to run our integration tests, and failed to find its dependencies (I could not find the rpm python bindings, at the very least). Any pointers on how to succeed with that?

@omajid

This comment has been minimized.

Contributor

omajid commented Mar 29, 2017

Thanks for the suggestions!

I made the id change locally and I think the result looks slightly worse. Ids can often be very long (for example buildarch-instead-of-exclusive-arch) and even the shortest of them are trimmed in the ID column in the error list. The description in the minibuffer also suffers. In the example above, it goes from

invalid-url Source0: foobar.tar.xz

to

Source0: foobar.tar.xz [invalid-url]

Which seems harder to parse to me. What do you think? Is it worth it to make the id change?

As for building rpmlint from source, I am not 100% sure. I use Fedora where it's prebuilt. Looking at the build scripts, it looks like rpmlint needs rpm-python and rpm-python is built as part of rpm. The invocation for building rpm-python looks like ./configure followed by python2 setup.py build and python3 setup.py build.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Mar 29, 2017

@omajid Thanks for trying it out. You are right: the loss of legibility is not worth it. LGTM then 👍

And thanks for the hints for building rpmlint; hopefully I will get to the bottom of it ;)

@fmdkdd

One last thing: could you add a line under New features into CHANGES.rst documenting this change?

Add an error-explainer for rpm-rpmlint
rpmlint's warnings/errors are very terse by default. For example, if
the file contains the following:

    Source0: foobar.tar.xz

The warning shown by flycheck is:

    invalid-url Source0: foobar.tar.xz

The explainer helps clarify the root cause:

    invalid-url:
    The value should be a valid, public HTTP, HTTPS, or FTP URL.

@omajid omajid force-pushed the omajid:rpm-lint-error-explainer branch from cbda8e1 to c71c92a Apr 1, 2017

@omajid

This comment has been minimized.

Contributor

omajid commented Apr 6, 2017

Hey, does this updated patch look okay?

@fmdkdd

fmdkdd approved these changes Apr 6, 2017

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Apr 6, 2017

@omajid It does! Looks like I missed your commit. Thanks for reminding me :)

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Apr 6, 2017

Hmm looks like the CLA hook is bugged out due to a cosmetic change (they changed "licence/cla" to "license/cla"...).

@omajid Could you push again to force the checks to re-trigger? I can't merge otherwise.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Apr 11, 2017

@omajid The CLA issue has been fixed. No need to push.

Thank you very much for this!

@fmdkdd fmdkdd merged commit 9d40b3e into flycheck:master Apr 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment