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

Added option to follow external sources in shellcheck #1256

Merged
merged 5 commits into from May 25, 2017

Conversation

@CeleritasCelery
Copy link
Contributor

commented May 24, 2017

Added the option to follow external sources in Shellcheck as requested in #1011. Created a Flycheck option variable which was mentioned in #1251 and missing from #1254. Fixed from #1255.

@CeleritasCelery

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2017

Apparently it is complaining because the option is true by default. Is that not allowed? That seem to be the best way to implement it.
Expected (flycheck-shellcheck-follow-sources) to `equal' nil

flycheck.el Outdated
(e.g. # shellcheck source=/full/path/to/file.txt)."
:type 'boolean
:safe #'booleanp
:package-version '(flycheck . "0.21"))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel May 24, 2017

Member

That version number seems wrong.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented May 24, 2017

I think it's complaining about missing documentation, not about the default value. Not sure why, though, since you did include docs. Do you have time to look into the documentation test and figure out why it's failing? Otherwise I'll look, but I might not have time today.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented May 24, 2017

Oh, I know, sorry — you're missing docs for this option in the manual, I believe.

CeleritasCelery added some commits May 24, 2017

@CeleritasCelery

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2017

All right @cpitclaudel, I added the doc and corrected the version.

flycheck.el Outdated
@@ -9549,7 +9549,7 @@ or added as a shellcheck directive before the source command
(e.g. # shellcheck source=/full/path/to/file.txt)."
:type 'boolean
:safe #'booleanp
:package-version '(flycheck . "0.21"))
:package-version '(flycheck . "0.31"))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel May 25, 2017

Member

31, not 0.31 :)

@cpitclaudel
Copy link
Member

left a comment

LGTM, thanks :)

@cpitclaudel cpitclaudel merged commit 0a62ea2 into flycheck:master May 25, 2017

2 checks passed

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

This comment has been minimized.

Copy link

commented Jul 25, 2017

@cpitclaudel apt installs shellcheck 0.3.7, that does not support this yet.
See https://emacs.stackexchange.com/questions/34337/flycheck-gives-errors-when-i-use-shellcheck-as-linter-for-bash-scripts/34398#34398

it was first introduced here
https://github.com/koalaman/shellcheck/blob/v0.4.0/shellcheck.hs

apt install
[ cescoferraro@desktop ] ~/go/src/github.com/cescoferraro/react-boil
$ shellcheck --version
ShellCheck - shell script analysis tool
version: 0.3.7
license: GNU Affero General Public License, version 3
website: http://www.shellcheck.net

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Jul 25, 2017

Okay, so --external-sources option is only in shellcheck 0.4.0 and up. We don't make extra effort for maintaining backwards compatibility. Debian stable ships with 0.4.4, and that's one of our reference point.

Other than installing a previous version of Flycheck, you can turn this particular option to nil, and the flag won't be added to the call.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

Should this have been documented in CHANGES, btw?

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

@cpitclaudel New options usually go to CHANGES, yes. Want to make a PR/commit?

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

@cpitclaudel Which version of Flycheck was this fixed in?

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

I don't think we've made a release since this option was added. This was merged in May, and our last release is from October of last year.

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

You said in #1313 (comment)

Can you try again on the latest Flycheck (from MELPA)? I think your problem should be fixed there. (#1256)

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

Yes. I'm suggesting that you try with an unreleased version. Sorry if that wasn't clear :/

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

@cpitclaudel Ok. Well, I'll keep an eye open for the next release.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

Noted. Just to be sure that we didn't do something wrong with this PR: you use MELPA Stable instead of the usual MELPA, and you're not interested in trying with a more recent clone of Flycheck (using the usual MELPA or a fresh clone from Github); correct?

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

@cpitclaudel I reconfigured Emacs to use these repos a few weeks ago:

(setq-default package-archives
              '(
                ("MELPA Stable" . "https://stable.melpa.org/packages/")
                ("gnu"          . "http://elpa.gnu.org/packages/")
                ("marmalade"    . "http://marmalade-repo.org/packages/")
                ("melpa"        . "https://melpa.org/packages/")
                ("org"          . "http://orgmode.org/elpa/")
                ))

Do I have to remove

            ("MELPA Stable" . "https://stable.melpa.org/packages/")

to use this?

           ("melpa"        . "https://melpa.org/packages/")
@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

I'm not sure. I don't know much about MELPA stable; sorry :/

@Simplify

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

@davidhedlund Both will work together, but you will see many duplicated packages, just different versions. Packages from "melpa stable" will have normal version number, for example 0.1.1 or 31... Packages from "melpa" (unstable) will have version number in YYYYMMDD.X{1-4} format.

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

@Simplify Thanks. I'm using this in my .emacs:

;; Automatically install packages (Emacs 24, use inbuilt package-install-selected-packages for Emacs 25)

;; === CUSTOM CHECK FUNCTION ===
(defun ensure-package-installed (&rest packages)
  "Assure PACKAGES are installed, ask for installation if they are not."
  (mapcar
   (lambda (package)
     (unless (package-installed-p package)
       (package-install package)))
   packages)
  )

;; === List my packages ===
;; simply add package names to the list
(ensure-package-installed
 'flycheck
 ;; ... etc
 )

That will install flycheck from stable melpa, how do I add lines to install it from unstable melpa instead?

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

@Simplify The flycheck package is not duplicated when I mix stable and unstable MELPA repositories.

@Simplify

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

@davidhedlund Yes, when package is installed, you see only installed version (also archive column is empty for installed packages). Run M-x package-list-packages to list all installed and available packages. Package version 20170823.1732 is latest (melpa). If you see version 30, that means that you are using olmost a year stable version from melpa stable. Uninstall it. In that case you will see both versions of Flycheck, 30 and 20170823.1732. Install version 20170823.1732 to use package from melpa.

You can also run M-x flycheck-version. Message in echo area should be something like Flycheck version: 31snapshot (package: 20170823.1732) for melpa version. Package from melpa stable will be version 30 i guess.

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

@cpitclaudel MELPA Stable is referred to in http://www.flycheck.org/en/latest/user/installation.html (a link found in https://github.com/flycheck/flycheck). Here's my positive feedback to you: I think it would be good if you knew more about it so you can help people in this repository.

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

@Simplify Thanks. Yes, I simply had to update the package, it was old.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

Here's my positive feedback to you: I think it would be good if you knew more about it so you can help people in this repository.

Thanks for the suggestion :) I think it's a matter of time allocation: I know enough about MELPA to support installing Flycheck through it, but I do not have time to debug user configurations. I don't expect the Flycheck bug tracker to help with general Emacs questions: that's why the bug reporting guidelines ask contributors to try to reproduce the problem in emacs -Q.

Fortunately, we have plenty of talented people on the team who were happy to help track the problem! Thanks @Simplify.

@davidhedlund

This comment has been minimized.

Copy link

commented Aug 30, 2017

@cpitclaudel

Fortunately, we have plenty of talented people on the team who were happy to help track the problem! Thanks @Simplify.

That's right.

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