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 'perl-module-list' option #1207

Merged
merged 1 commit into from Feb 4, 2018

Conversation

@michaelblack
Copy link
Contributor

commented Feb 7, 2017

This just adds the ability to specify perl modules to use via the -M flag when syntax checking.

I ran make check unit specs LANGUAGE=perl integ without errors.

@CLAassistant

This comment has been minimized.

Copy link

commented Feb 7, 2017

CLA assistant check
All committers have signed the CLA.

@fmdkdd

fmdkdd approved these changes Feb 7, 2017

Copy link
Member

left a comment

Great PR! Thank you for this contribution.

LGTM except the variable version number and a minor question in the tests.

@cpitclaudel?

flycheck.el Outdated
string is a module to 'use' in Perl."
:type '(repeat :tag "Module")
:safe #'flycheck-string-list-p
:package-version '(flycheck . "30.0"))

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Feb 7, 2017

Member

The version would be "31" since "30" has already been released, and we dropped decimals since version 26.

'(4 nil error "Global symbol \"$dependency_b\" requires explicit package name (did you forget to declare \"my $dependency_b\"?)"
:checker perl))
;; Including those modules should allow them to check
(cl-letf

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Feb 7, 2017

Member

It did not occur to me we could call -should-syntax-check multiple times in the same test. Is it just to regroup the checks under the same 'module' test? Or are there any other benefits? (Just curious; it's fine as is)

Why cl-letf though? Wouldn't a simple let would suffice to override the variable for the next call to -should-syntax-check?

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Feb 7, 2017

Oh and I guess the line in CHANGES.rst should be moved to New features.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

Thanks; the code looks good (and thanks @fmdkdd for the review). I don't speak much Perl; can you explain the rationale for this change? In which cases would you set this variable (and what are the alternatives?)

@michaelblack

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

@fmdkdd Fixed the issues.
There was no reason for me to be using cl-letf over let and looking at the style guide, it looks like use of cl-lib is discouraged, whoops!
As for the multiple -should-syntax-check, yep, that was just for grouping multiple tests with different let requirements under the same 'module' test.

@cpitclaudel
This change allows you to import modules before checking the code. An example is given in the test files where the script refers to perl variables in the modules that are made to be imported through setting -perl-module-list. There is no real alternative to the option other than aliasing perl or pointing it to an equivalent executable that runs the -M flag or explicitly including the module in your code. Though adding it to each piece of code is not really an alternative in workflows that expect this option to be available.
As for an example use case, at my company we pass in a module that does complicated path setup based on environment variables.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

@michaelblack Thanks; does that mean that when you run the script itself you also pass it these modules? The docs suggest that "use module" is the same as passing -Mmodule; can you clarify in which cases using -M is better (or more convenient)? Finally, do other IDEs have a similar option?

I'm a bit worried about this kind of options, because they tend to be too specific to Flycheck. For example, can your script currently be run in cperl-mode? Should we really add this setting to Flycheck, or should we instead add it to e.g. cperl-mode?

@michaelblack

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

@cpitclaudel

... does that mean that when you run the script itself you also pass it these modules

Yep. You would, and if you are expecting that to be the case, then you would have to do something awkward such as include it in the file for the purposes of flycheck then removing the line when actually running in order to avoid duplicate imports. This is especially true in the case where there are scripts wrapping perl to pass the -M option with no real way to change it. As for cases when you are running perl directly, in terms of best practices for passing -M or directly useing it, for the case of a script I would probably argue it's better practice to use it (though many workflows don't follow best practice). However for things more complicated than a script (such as a large application code base based on mod_perl (~40k files in my case)) there is a convenience factor in passing it in as -M to have your own sort of "prelude" code.

... do other IDEs have a similar option?

I can't speak to that as I haven't used other perl IDEs (and when looking around, the ones that do exist have sparse documentation). For what it's worth though, I have seen this option passed into syntax checkers rather commonly in vim, where arbitrary arguments to the executable are allowed.

... can your script currently be run in cperl-mode? Should we really add this setting to Flycheck, or should we instead add it to e.g. cperl-mode?

Well cperl can't replace flycheck. If the flycheck executable is set to perl without the modules, it will still report invalid errors, such as invalid modules, variables, and subroutines (and potentially much more given perl's somewhat horrifying ability to modify syntax on module import).

@michaelblack

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2017

Hey @cpitclaudel
Just wondering about an update on this and if I answered your questions.

Also on this

Should we really add this setting to Flycheck, or should we instead add it to e.g. cperl-mode

I realized I may have misunderstood. Were you talking about potentially adding a custom variable for those modes and introducing a dependency on that?

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Mar 9, 2017

Hi @michaelblack

Sorry for the delay. I think you make a valid case for this setting. Sorry for being unclear about the best location for this setting. I was wondering whether cperl or other modes or packages provided any feature that would require running perl, and would benefit from knowing which flags to run perl with. If not, then it makes sense to add the setting here. On the other hand, if that setting might be useful to another package as well, then it makes sense to add it to e.g. cperl-mode.

Other than this, I'm happy with the implementation :) Thanks!

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

@cpitclaudel So, does it looks good to you? I've re-read the discussion and I don't really understand what you mean by "adding it to another package". If the setting was available in cperl-mode, it wouldn't make a difference for the perl checker in flycheck, would it?

@michaelblack In any case, could you rebase your branch on top of master?

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

So, does it looks good to you?

Yup

I've re-read the discussion and I don't really understand what you mean by "adding it to another package". If the setting was available in cperl-mode, it wouldn't make a difference for the perl checker in flycheck, would it?

My point was that if the user wants to run Perl for Flycheck with a given flag, they probably want other instances of Perl started from Emacs on the same file to use that flag too. Given this, it'd make more sense to have the variable controlling that flag defined in, say, cperl-mode, and then using that variable from Flycheck.

But that's minor; I'd rather see this merged than wait longer :) If cperl-mode gains a similar option, users can easily make the flycheck one an alias of the cperl one.

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

@cpitclaudel Now I do get it. Thanks for taking the time to explain :)

@michaelblack All good then, you just have to rebase and we'll happily merge!

@cpitclaudel cpitclaudel force-pushed the michaelblack:perl-module-list branch from 5f26a49 to 51b762c Feb 4, 2018

@cpitclaudel cpitclaudel force-pushed the michaelblack:perl-module-list branch from 51b762c to c3f4a5e Feb 4, 2018

@cpitclaudel cpitclaudel merged commit 5e745ba into flycheck:master Feb 4, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

Rebased and merged :)

@michaelblack michaelblack deleted the michaelblack:perl-module-list branch Mar 8, 2018

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