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

Introduce TravisCI + Small perlcritic changes #2

Merged
merged 11 commits into from Aug 27, 2016

Conversation

Projects
None yet
2 participants
@lancew
Contributor

lancew commented Aug 26, 2016

I am not sure how to break the first two commits from the rest (or squash perhaps), sorry.

First two commits address some minor perlcritic errors.
Remaining ones enable TravisCI testing.

Done as part of CPAN Pull Request Challenge. Feedback appreciated.

lancew added some commits Aug 26, 2016

Change to eval{}
Resolves perlcritic failure "Expression form of "eval"..."
Modernise file opens in Makelfile.pl
Change file opens to 3 arg file opens.
Use variables for filehandler not barewords.

This resolves issues flagged by perlcritic level 5(Gentle).
Add .travis.yml
This will cause this repo to be tested automatically on TravisCI
Update travis config
Remove the coveralls part from travis config.
Tell travis to use makefile
@shlomif

This comment has been minimized.

Show comment
Hide comment
@shlomif

shlomif Aug 26, 2016

Owner

Thanks, Lance! I'll take a look.

Owner

shlomif commented Aug 26, 2016

Thanks, Lance! I'll take a look.

@shlomif

This comment has been minimized.

Show comment
Hide comment
@shlomif

shlomif Aug 27, 2016

Owner

Hi Lance!

Some comments on your pull-request:

  1. In t/*.t eval { use MyMod; } instead of eval "use MyMod;" is wrong and will unconditionally try to load the module and throw an exception if it isn't (that's because use is compile-time):
shlomif@telaviv1:~/progs/perl/cpan/Math/Math-Cephes$ perl -E 'eval { use NonExist; }'
Can't locate NonExist.pm in @INC (you may need to install the NonExist module)
.
.
.
shlomif@telaviv1:~$ perl -E 'eval { require NonExist; }'
shlomif@telaviv1:~$ 

so either put require and/or "require; import" there or keep it as string eval.
1.

-        print MCONF fix_mconf($def, $defs{$def});
+        print $mconf_fh fix_mconf($def, $defs{$def});

print $fh @args is better written as print {$fh} @args (see http://perl-begin.org/tutorials/bad-elements/#print_to_fh ). This is a relatively minor problem.


Please fix these problems and let me know when I can review your changes again.

Owner

shlomif commented Aug 27, 2016

Hi Lance!

Some comments on your pull-request:

  1. In t/*.t eval { use MyMod; } instead of eval "use MyMod;" is wrong and will unconditionally try to load the module and throw an exception if it isn't (that's because use is compile-time):
shlomif@telaviv1:~/progs/perl/cpan/Math/Math-Cephes$ perl -E 'eval { use NonExist; }'
Can't locate NonExist.pm in @INC (you may need to install the NonExist module)
.
.
.
shlomif@telaviv1:~$ perl -E 'eval { require NonExist; }'
shlomif@telaviv1:~$ 

so either put require and/or "require; import" there or keep it as string eval.
1.

-        print MCONF fix_mconf($def, $defs{$def});
+        print $mconf_fh fix_mconf($def, $defs{$def});

print $fh @args is better written as print {$fh} @args (see http://perl-begin.org/tutorials/bad-elements/#print_to_fh ). This is a relatively minor problem.


Please fix these problems and let me know when I can review your changes again.

lancew added some commits Aug 26, 2016

Modernise file opens in Makelfile.pl
Change file opens to 3 arg file opens.
Use variables for filehandler not barewords.

This resolves issues flagged by perlcritic level 5(Gentle).
Revert the eval{} to eval ""
As per feedback, the eval{} was wrong and will unconditionally try
to load the module and throw an exception if it isn't (because use
is compile-time).
@lancew

This comment has been minimized.

Show comment
Hide comment
@lancew

lancew Aug 27, 2016

Contributor

Thanks for the feedback.
I have put the evals back as they word and updated the filehandle prints.
Lance

Contributor

lancew commented Aug 27, 2016

Thanks for the feedback.
I have put the evals back as they word and updated the filehandle prints.
Lance

@shlomif shlomif merged commit 5135c30 into shlomif:master Aug 27, 2016

@shlomif

This comment has been minimized.

Show comment
Hide comment
@shlomif

shlomif Aug 27, 2016

Owner

Thanks for your contribution! I merged your changes now.

Owner

shlomif commented Aug 27, 2016

Thanks for your contribution! I merged your changes now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment