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

Add option to avoid showing other files' errors #1579

Merged
merged 2 commits into from Jun 12, 2019

Conversation

ibizaman
Copy link
Contributor

@ibizaman ibizaman commented Jun 6, 2019

Since showing errors from other files was added recently, I'm adding an option to revert back to not showing them.

Note I tried to look at my changes in the documentation but the make html-auto step failed with the same error that appeared recently(?) https://travis-ci.org/flycheck/flycheck/jobs/531994058

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Jun 6, 2019

CLA assistant check
All committers have signed the CLA.

@fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Jun 9, 2019

Thanks for the patch! LGTM.

The Travis failure is surprising indeed. I've found the culprit: the file
http://mirror.keystealth.org/gnu/texinfo/htmlxref.cnf has a type on line 175:

 eww		node	$(EMACS}/html_node/eww/

The $(EMACS} should be ${EMACS}. This makes the python string template panic.

@cpitclaudel Do you know who to contact to fix this?

CHANGES.rst Outdated Show resolved Hide resolved
:group 'flycheck
:type 'boolean
:package-version '(flycheck . "32")
:safe #'booleanp)
Copy link
Member

@cpitclaudel cpitclaudel Jun 10, 2019

Choose a reason for hiding this comment

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

I wonder if a -p suffix would be appropriate, since this is a boolean flag. Additionally, the name is a bit of a mouthful, but maybe that's justified by the consistency with -minimum-level?

Copy link
Contributor Author

@ibizaman ibizaman Jun 11, 2019

Choose a reason for hiding this comment

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

I must agree with the mouthful 😅

Another option could be to have add a new level to flycheck-relevant-error-other-file-minimum-level called 'never. Having only one option could avoid confusion.

Btw, I'm not totally sure it should have that -p suffix. I can definitely be wrong but I thought a predicate was only a function while here it's variable, right?

Copy link
Member

@cpitclaudel cpitclaudel Jun 11, 2019

Choose a reason for hiding this comment

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

Another option could be to have add a new level to flycheck-relevant-error-other-file-minimum-level called 'never. Having only one option could avoid confusion.

That could work, but it will conflict if someone defines an error level called never. That would be weird, of course.

Btw, I'm not totally sure it should have that -p suffix. I can definitely be wrong but I thought a predicate was only a function while here it's variable, right?

No, I think you're right, actually :)

Copy link
Contributor Author

@ibizaman ibizaman Jun 11, 2019

Choose a reason for hiding this comment

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

And what about setting flycheck-relevant-error-other-file-minimum-level to nil means never show errors from other files? I think that would make sense.

Otherwise I'm totally up to renaming flycheck-relevant-error-other-file-show to something else. I think we should keep the flycheck-relevant-error-other-file prefix, to make it obvious it's "paired" with flycheck-relevant-error-other-file-minimum-level. I'm not the best at naming things though.

Copy link
Member

@cpitclaudel cpitclaudel Jun 11, 2019

Choose a reason for hiding this comment

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

And what about setting flycheck-relevant-error-other-file-minimum-level to nil means never show errors from other files? I think that would make sense.

It could, but I think that value is already used to display all errors from other files. Let's go with the original name. I'll merge as soon as CI is back on track.

@cpitclaudel
Copy link
Member

@cpitclaudel cpitclaudel commented Jun 11, 2019

@fmdkdd I think the issue should be fixed soon. I reported the bug to the texinfo folks, and it was fixed today. An updated version should be up soon.

fmdkdd
fmdkdd approved these changes Jun 12, 2019
@fmdkdd fmdkdd merged commit b91ad8a into flycheck:master Jun 12, 2019
2 checks passed
@ibizaman
Copy link
Contributor Author

@ibizaman ibizaman commented Jun 12, 2019

Thank you both for your review! That was a very pleasant experience. And the on-boarding documents are very well done.

@fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Jun 13, 2019

@ibizaman Thank you for the patch. Always happy to have good contributions 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants