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 perlcritic to Travis with gentle severity #6294

Merged
merged 5 commits into from Nov 30, 2018

Conversation

jknphy
Copy link
Contributor

@jknphy jknphy commented Nov 27, 2018

In order to add perlcritic to Travis (run succesfully make perlcritic in our ci) and start to introduce some new policies we need:

  • First, apply default policy (empty means --severity gentle or --severity 5 according documentation) and modify tests accordingly. This are potential modification in files needed to successful pass a default call to make perlcritic.
    I had to exclude for the moment policy ProhibitStringyEval as I could not resolve v1 and v2 simply replacing them with { } due to in turn makes `make test' failing on importing modules.
  • Second, include HashKeyQuote perlcritic policy(one of the not used existing policies in the original makefile) and modify files accordingly.
  • Third, do the same for the other existing policy ConsistentQuoteLikeWords.
  • Finally, add new policy Perl::Critic::Policy::ControlStructures::ProhibitDeepNests and use a configuration .perlcriticrc and setup max_nests where does not fail (fixing one level)
  • Besides as an enhancement git grep is not producing any visual data except for the unnecessary user intervention so I added the option to be silent. Also I realized that we should not check os-autoinst as it is a different repo.

Previous steps are reflected in different commits in case something potentially could go south.

Related ticket

https://progress.opensuse.org/issues/44165

@jknphy jknphy force-pushed the add_perlcritic_travis branch 3 times, most recently from 94660a8 to 6116761 Compare November 27, 2018 09:59
@jknphy jknphy changed the title [WIP] Add perlcritic to Travis Add perlcritic to Travis Nov 27, 2018
@jknphy jknphy changed the title Add perlcritic to Travis Add default perlcritic (gentle severity) to Travis Nov 27, 2018
@jknphy jknphy changed the title Add default perlcritic (gentle severity) to Travis Add perlcritic (gentle severity) to Travis Nov 27, 2018
@jknphy jknphy changed the title Add perlcritic (gentle severity) to Travis Add perlcritic to Travis with gentle severity Nov 27, 2018
@jknphy jknphy changed the title Add perlcritic to Travis with gentle severity [WIP] Add perlcritic to Travis with gentle severity Nov 27, 2018
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

I wonder with what change we lost perlcritic checks to be executed against each PR in the past …

.travis.yml Outdated Show resolved Hide resolved
@jknphy jknphy force-pushed the add_perlcritic_travis branch 3 times, most recently from 6736caf to 657b53a Compare November 28, 2018 07:52
@jknphy jknphy changed the title [WIP] Add perlcritic to Travis with gentle severity Add perlcritic to Travis with gentle severity Nov 28, 2018
@jknphy
Copy link
Contributor Author

jknphy commented Nov 28, 2018

There is a call to perlcritic in target test-merge in Makefile and it fulfills its purpose in user's local env but I don't see we run this in Travis. My best guess is that the list of changed files given by
git diff --name-only FETCH_HEAD `git merge-base FETCH_HEAD master` is not returning anything and it does in local env.
According with Travis docs "we build the merge between the source branch and the upstream branch". That could be a possible reason why it is not tracking the differences between both branches. I've no idea if this worked in the past as it is hard to track travis logs from a lot of time ago.

@jknphy jknphy force-pushed the add_perlcritic_travis branch 2 times, most recently from 5c03882 to 5adfe76 Compare November 29, 2018 10:12
Copy link
Member

@rwx788 rwx788 left a comment

Choose a reason for hiding this comment

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

LGTM. Do you have any example of the output if such check fails? As we don't want to confuse people.

@okurz
Copy link
Member

okurz commented Nov 29, 2018

@coolo do you have an idea regarding #6294 (review) and #6294 (comment) ?

@coolo
Copy link
Contributor

coolo commented Nov 29, 2018

this is 2 years old - it's not too unlikely travis changed something in between :)

@jknphy
Copy link
Contributor Author

jknphy commented Nov 29, 2018

@rwx788 for example if we would activate policy [BuiltinFunctions::ProhibitStringyEval] in .perlcriticrc removing the leading - user would get this output:

make perlcritic                                  
PERL5LIB=tools/lib/perlcritic:$PERL5LIB perlcritic --quiet --gentle --exclude=os-autoinst ./tests ./products ./lib
lib/main_common.pm: Expression form of "eval" at line 262, column 5.  See page 161 of PBP.  (Severity: 5)
lib/main_common.pm: Expression form of "eval" at line 265, column 9.  See page 161 of PBP.  (Severity: 5)
make: *** [Makefile:93: perlcritic] Error 2

With a quick google search of the kind perlcritic Expression form of "eval" should lead him/her to specific policy doc in CPAN

.perlcriticrc Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@okurz
Copy link
Member

okurz commented Nov 29, 2018

@kraih WDYT?

@kraih
Copy link
Member

kraih commented Nov 30, 2018

@okurz My plan for openQA and os-autoinst is to use Perl::Critic::Freenode with severity 4 (--stern). If that makes sense for this repo i'm not sure yet, a first scan looks like this.

@okurz okurz merged commit e70ba1c into os-autoinst:master Nov 30, 2018
@okurz
Copy link
Member

okurz commented Nov 30, 2018

@kraih I see. I plan to extend the perl critic checks later and mentioned your comment in https://progress.opensuse.org/issues/44075#note-2

@jknphy jknphy deleted the add_perlcritic_travis branch January 22, 2019 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants