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

Fix a couple of linting issues #35

Merged
merged 4 commits into from
Nov 17, 2017
Merged

Conversation

fbergroth
Copy link
Contributor

No description provided.

@fbergroth fbergroth mentioned this pull request Nov 16, 2017
@fbergroth
Copy link
Contributor Author

Oops, missed a couple, hold on.

@fbergroth
Copy link
Contributor Author

Ok, good to go.

@jojojames
Copy link
Collaborator

I'm curious about the keyword replacements? Would adding (vs. replacing) extra keywords have fixed the linting issues?

@fbergroth
Copy link
Contributor Author

Indeed, my bad. I'll update it.

@jojojames jojojames merged commit 47b1e57 into emacs-evil:master Nov 17, 2017
@jojojames
Copy link
Collaborator

Thanks! Nice, one step closer towards Melpa.

@@ -106,7 +106,7 @@
(kbd "<down-mouse-1>") 'pdf-view-mouse-set-region

(kbd "C-c C-c") 'docview-mode
(kbd "C-c <tab>") 'pdf-view-extract-region-image
[?\C-c tab] 'pdf-view-extract-region-image ; workaround package-lint bug
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should go down the route of twisting the code to make package-lint happy. package-lint is a helper, it has false positives and it's fine: we should not make the code uglier just for the sake of it.

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 crashed package-lint, and it would not continue it's analysis without the fix. This is not about silencing false positives.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, but it is still just a helper. If the helper has an issue, it should not change our code which is fine.

Is the issue reported upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine, just revert the line and wait for it to be fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not take lines in the sand. :)

We can both keep the code and report it upstream at the same time.

Packagelint is a tool but a very useful one. It's good to play nice with it even if it means tweaking the code just a little.

Think // ignore-linter type lines in other languages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, we can keep the line and revert once it's fixed.
Found the issue: purcell/package-lint#96
@fbergroth : thanks for reporting.

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.

3 participants