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.

Sign up for GitHub

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

Jump to bottom

Add perlcritic freenode #7231

Merged
merged 7 commits into from May 13, 2019
Merged

Add perlcritic freenode #7231

merged 7 commits into from May 13, 2019

Conversation

jknphy
Copy link
Contributor

@jknphy jknphy commented Apr 9, 2019 •
edited

Use Perl::Critic::Freenode perlcritic rules as in os-autoinst and openQA.

@jknphy jknphy force-pushed the perl_critic_freenode branch 2 times, most recently from 398c71c to 30e7b07 Compare April 9, 2019 08:06
@jknphy jknphy changed the title [WIP] Perl critic freenode [WIP] Add perlcritic freenode Apr 9, 2019
@jknphy jknphy force-pushed the perl_critic_freenode branch 13 times, most recently from c02e2b8 to 6ed93a9 Compare May 2, 2019 14:31
asdil12
asdil12 reviewed May 2, 2019
View reviewed changes
cpanfile Outdated Show resolved Hide resolved
@jknphy jknphy force-pushed the perl_critic_freenode branch 6 times, most recently from d683db9 to 7d527de Compare May 3, 2019 05:47
@jknphy jknphy changed the title [WIP] Add perlcritic freenode Add perlcritic freenode May 3, 2019
@jknphy
Copy link
Contributor Author

jknphy commented May 9, 2019

@coolo @asdil12 feel free to merge it if I addressed your points, so we can continue adding more checks in the future.

asdil12
asdil12 approved these changes May 9, 2019
View reviewed changes
Copy link
Member

@asdil12 asdil12 left a comment

Choose a reason for hiding this comment

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

LGTM but I think someone else should have a look before we merge this.

@jknphy jknphy requested a review from Martchus May 9, 2019 12:57
@jknphy
Copy link
Contributor Author

jknphy commented May 9, 2019

Thanks @asdil12 , it comes to my mind @Martchus who implemented similar approach for openQA and os-autoinst.

Martchus
Martchus approved these changes May 9, 2019
View reviewed changes
Copy link
Contributor

@Martchus Martchus left a comment •
edited

Choose a reason for hiding this comment

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

Looks good - although it was @kraih who did similar changes for os-autoinst and openQA.

I'm only wondering whether having 76 open PRs makes this a good time to merge. On the other hand that number isn't likely to decrease soon.

@jknphy
Copy link
Contributor Author

jknphy commented May 9, 2019

Ahm right, I got confused with who authored previous work. I have been rebasing this work for some days, but yes, it should not change that number too much. Feel free to merge it in the best moment any of you can find. Thanks.

@asdil12
Copy link
Member

asdil12 commented May 10, 2019

Then rebase your changes and we will merge.

@jknphy jknphy force-pushed the perl_critic_freenode branch 2 times, most recently from b166793 to 0ad7c80 Compare May 10, 2019 15:01
@jknphy
Copy link
Contributor Author

jknphy commented May 10, 2019

Changes rebased.

@asdil12 asdil12 merged commit 42f1f77 into os-autoinst:master May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coolo coolo coolo left review comments

@asdil12 asdil12 asdil12 approved these changes

@Martchus Martchus Martchus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jknphy @asdil12 @coolo @Martchus