Skip to content

Avoid using $_ when loading the commands from namespaces - #2181

Open
bbrtj wants to merge 1 commit into
mojolicious:mainfrom
bbrtj:main
Open

Avoid using $_ when loading the commands from namespaces#2181
bbrtj wants to merge 1 commit into
mojolicious:mainfrom
bbrtj:main

Conversation

@bbrtj

@bbrtj bbrtj commented May 17, 2024

Copy link
Copy Markdown

Summary

Solution proposed in #2175

Motivation

Polluting $_ can happen deep down in dependencies tree of the loaded command.

References

resolves #2175

@bbrtj
bbrtj force-pushed the main branch 2 times, most recently from f6c8a1f to 91c03a9 Compare May 17, 2024 19:15
@kraih
kraih requested review from a team, jberger, jhthorsen, kraih and marcusramberg September 10, 2024 14:47
@kraih
kraih requested a review from Copilot June 16, 2025 16:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the command-loading loop to avoid using the implicit $_, preventing unintended global variable pollution.

  • Replaced a chained grep/for using $_ with an explicit for my $pkg loop.
  • Added an early next unless _command($pkg) guard.
  • Updated the hash assignment to use the named $pkg variable.
Comments suppressed due to low confidence (3)

lib/Mojolicious/Commands.pm:68

  • [nitpick] Consider renaming $pkg to a more descriptive name like $module or $command_class to clarify that it represents a command class.
for my $pkg (find_modules($ns), find_packages($ns)) {

lib/Mojolicious/Commands.pm:67

  • [nitpick] Add a comment explaining that this loop avoids using $_ to prevent pollution from upstream code, clarifying the purpose of the refactoring.
for my $ns (@{$self->namespaces}) {

lib/Mojolicious/Commands.pm:71

  • Consider adding a test case to verify that command loading no longer pollutes $_, ensuring this transformation addresses the original issue (#2175).
}

@bbrtj

bbrtj commented Dec 9, 2025

Copy link
Copy Markdown
Author

Bump, why isn't this merged? Seems like a low-hanging fruit.

@kraih

kraih commented Dec 9, 2025

Copy link
Copy Markdown
Member

Nobody will even look at it before tests passed,

@bbrtj

bbrtj commented Dec 9, 2025

Copy link
Copy Markdown
Author

I'm confused. Am I required to take any action here? Did the tests fail? They look like they never ran at all.

@kraih

kraih commented Dec 9, 2025

Copy link
Copy Markdown
Member

No idea what went wrong i'm afraid, and nothing we can do about it.

@bbrtj

bbrtj commented Dec 9, 2025

Copy link
Copy Markdown
Author

I rebased to latest main. Seems like workflows await approval now. Maybe that'll fix them.

@bbrtj

bbrtj commented Dec 10, 2025

Copy link
Copy Markdown
Author

Most tests passed now. Windows test seems to be failing in other PRs too, regardless of their content.

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.

Commands error when topic variable set to undef in compilation

3 participants