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

Unify Perl Critic #32

Merged
merged 1 commit into from Jan 14, 2024
Merged

Conversation

josegomezr
Copy link
Contributor

  • Introducing tools/perlcritic an improved wrapper over perl's perlcritic.

    It automatically appends this project's policies that are defined under the openqa theme.

  • Adds a complementary GitHub Action to run tools/perlcritic automatically on Pull Requests & Master.

  • Fixed perlcritics complaints.

Branched off #30.

# This is not our code, and formatting should stay the same for
# better comparison with new versions of B::Deparse
# <---- PATCH
package B::Deparse;
no warnings 'redefine';
no strict 'refs';

*{"B::Deparse::walk_lineseq"} = sub {
*{'B::Deparse::walk_lineseq'} = sub {
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment above says "This is not our code, and formatting should stay the same for better comparison with new versions of B::Deparse" so changing the formatting here seems wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The quote type changes are irrelevant to the inner workings of the code. It'll perform exactly the same task regardless of the quote.

Why keeping it out-of-style?

Copy link
Contributor

Choose a reason for hiding this comment

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

"for better comparison with new versions of B::Deparse" - although I'm just quoting the comment here. I guess I personally don't care much about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It says so in the comment: for better comparison with new versions of B::Deparse.
I can take a new version and do a diff to this snippet to see if anything has changed.
I just checked again, nothing has changed.
I wonder how much work it would be to create a reproducer for a bugreport. It's annoying to keep this patch around.

If it's easier, just exclude this file completely from linting instead of excluding certain lines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@josegomezr Be sure to resolve comments, or otherwise respond if you consider the matter addressed. Otherwise it looks like there's an open question here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just unified tabs to spaces, does that also constitute a unacceptable change as per:

"This is not our code, and formatting should stay the same for better comparison with new versions of B::Deparse"

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, tabs to spaces is a change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to confirm: tabs to spaces is not acceptable for this file in particular?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That part of the code should stay exactly the same. Only in this file, for the reason stated in the comment.

tools/perlcritic Outdated Show resolved Hide resolved
Copy link
Contributor

mergify bot commented Dec 15, 2023

This pull request is now in conflicts. Could you fix it? 🙏

Copy link
Collaborator

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

@josegomezr josegomezr force-pushed the add_perl_critic branch 2 times, most recently from d91c023 to 8b7d4d4 Compare December 15, 2023 14:42
@josegomezr
Copy link
Contributor Author

Ping (?)

@josegomezr
Copy link
Contributor Author

Ping x2 (?)

- Introducing `tools/perlcritic` an improved wrapper over
  perl's `perlcritic`.

  It automatically appends this project's policies that are
  defined under the `openqa` theme.

- Adds a complementary GitHub Action to run `tools/perlcritic`
  automatically on Pull Requests & Master.

- Fixed `perlcritics` complaints.

Branched off os-autoinst#30.

Co-authored-by: Oliver Kurz <okurz@suse.de>
Co-authored-by: Martchus <martchus@gmx.net>
@mergify mergify bot merged commit 6e38846 into os-autoinst:master Jan 14, 2024
4 checks passed
@perlpunk
Copy link
Collaborator

perlpunk commented Jan 18, 2024

os-autoinst/os-autoinst#2443
os-autoinst/openQA#5430
This PR might have to be reverted if we can't fix it soon.
It's not super urgent but it will block other updates.

edit: all good now, we fixed a couple of things.
This repo should run the actual checks on itself.

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

5 participants