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

Fixed a crash case and added an exclude optional parameter #1

Closed
wants to merge 1 commit into from

Conversation

rgeronimi
Copy link

-Fixed a crash (panic) case where the files in arguments do not exist. Instead, a clean error is back-propagated now
-Added an exclude optional argument to be able to skip temporary lines (e.g., TODO)

@walle
Copy link
Owner

walle commented Apr 5, 2016

Thank you! I'll merge this now. Github is currently experiencing problems,
but I have your code as a remote locally and am looking it over/merging
now. We will see when the changes are visible on Github, i.e. when they
have resolved their connectivity issues.

On Mon, Apr 4, 2016 at 9:06 PM, Raphael Geronimi notifications@github.com
wrote:

-Fixed a crash (panic) case where the files in arguments do not exist.
Instead, a clean error is back-propagated now
-Added an exclude optional argument to be able to skip temporary lines

(e.g., TODO)

You can view, comment on, or merge this pull request online at:

#1
Commit Summary

  • Fixed a crash case and added an exclude optional parameter

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1

@rgeronimi
Copy link
Author

Ah I am sorry I didn't notice (and thus, updated) the _test files, consequently the travis CI fails. You can update them, or I can do it easily, let me know.
Also, dont hesitate to rename the optional argument to follow your own conventions.
Thanks

walle added a commit that referenced this pull request Apr 5, 2016
If a path that does not exist is inputted the application crashed
with a null pointer panic.
As reported in #1.
This commit checks if the file info exists and if not outputs a user
friendly error message on stderr.
It does also check for any other error that could occur and outputs it
to stderr.
This commit also changes the signature of lll.ShouldSkip as the
error is not checked in the method.
walle added a commit that referenced this pull request Apr 5, 2016
Add the exclude flag as suggested in
#1.
The flag takes a regexp to match against lines. If the regexp is invalid
an error message is printed to stderr and the application exists
with status 1.
@walle
Copy link
Owner

walle commented Apr 5, 2016

Thank you for your contribution!

I took your changes, and made some modifications to them.

  • Only print an error on stderr and continue if a file is not found. (IMO the file not found error does not warrant an exit of the application, but a warning) Thanks for finding the panic!
  • Use a regexp instead of a string to make it possible to ignore more than one kind of line, e.g. TODO|FIXME (bad example since the code probably only uses one or the other) but you can still use it for only finding e.g. TODO.

I fixed the issue with the tests, and added some documentation to the README.

Thanks again!

@walle walle closed this Apr 5, 2016
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.

2 participants