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

Update the perl-linter's l:pattern to catch missing errors #2150

Merged
merged 3 commits into from
Dec 20, 2018

Conversation

mvgrimes
Copy link
Contributor

In some situations, errors reported by perl -c can have multiple
listings of "at line ". If the l:pattern is changed to
use non-greedy matching it will also match these.

For example:

use strict;
use DateTime;
$asdf=1;

Results in:

Global symbol "$asdf" requires explicit package name (did you forget to declare "my $asdf"?) at /Users/mgrimes/t.pl line 3, <DATA> line 1.
/Users/mgrimes/t.pl had compilation errors.

I am not 100% sure why perl -c generates errors with the extra "file
line ". It only happens in some versions of perl when certain
modules are used.

Where are the tests? Have you added tests? Have you updated the tests? Read the
comment above and the documentation referenced in it first. Write tests!

Seriously, read :help ale-development and write tests.

In some situations, errors reported by `perl -c` can have multiple
listings of "at <file> line <number>". If the l:pattern is changed to
use non-greedy matching it will also match these.

For example:
```
use strict;
use DateTime;
$asdf=1;
```

Results in:
```
Global symbol "$asdf" requires explicit package name (did you forget to declare "my $asdf"?) at /Users/mgrimes/t.pl line 3, <DATA> line 1.
/Users/mgrimes/t.pl had compilation errors.
```

I am not 100% sure why `perl -c` generates errors with the extra "file
line <num>". It only happens in some versions of perl when certain
modules are used.
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Update the tests if you want to capture more output. The changes here broke the tests for the existing example output we have tests for. You may have to update the pattern to cover all of the cases.

@mvgrimes
Copy link
Contributor Author

Sorry, that was the wrong patch. Not sure how that happened, but I've just pushed the correct patch. This passes run-tests on my machine.

@w0rp
Copy link
Member

w0rp commented Dec 17, 2018

Cool, that looks better. Add some more tests for the output we're able to capture now.

@w0rp w0rp merged commit 73ca1e7 into dense-analysis:master Dec 20, 2018
@w0rp
Copy link
Member

w0rp commented Dec 20, 2018

Cheers! 🍻

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

3 participants