Add an option to allow one to opt-out of adding empty import lists

[ThomasLamprecht]

While it might be OK by default to add empty import () statement to use statements that do not already import something so that they are future proofed against new default-exports it still adds a lot of noise and in bigger code bases that are under control by the same org it doesn't gains much value.

That's why I'd like to request a new option to opt-out of adding such "empty import statements" (or however they are called).

E.g., as of now the following line:

use Foo::Bar;

Always becomes

use Foo::Bar ();

With the newly proposed option that line should stay the same, at least if Foo::Bar is used at all.

Thanks for your consideration and for this module in general!

[oalders]

Thanks for this! My proposal would be to try to do this with the existing functionality. A few options:

use Foo::Bar; ## no perlimports

perlimports --ignore-module Foo::Bar ...

perlimports --create-config-file perlimports.toml

The config file gives you various options for default ignores:

ignore_modules                  = []
ignore_modules_filename         = ""
ignore_modules_pattern          = "" # regex like "^(Foo|Foo::Bar)"
ignore_modules_pattern_filename = ""

See https://metacpan.org/dist/App-perlimports/view/script/perlimports#-ignore-modules for a summary of these. The *_filename options are probably not so useful if you're already using a config file, but they are available. The would be more useful as command line switches, I think.

[ThomasLamprecht]

Thanks for your reply. The issue here is that we do not want to unconditionally ignore modules, just avoid adding the empty import by default for everything which is really not the same thing as ignoring them FWICT.

[oalders]

Can you give me some concrete examples of what perlimports is doing vs what you think it should be doing? Maybe a couple of diffs to illustrate the problem.

[mrsdizzie]

I think the issue is just wanting an option to not modify the line if it isn't explicitly importing anything by name. It isn't about any one particular model, its about preferring the format:

use Foo::Bar;

To

use Foo::Bar ();

Because the second one is not perceived to provide any extra information, looks less common, and perhaps even feels less clear.

In other words, this feels like a clear improvement on existing code clarity and intention:

-use HTTP::Request::Common;
+use HTTP::Request::Common qw( GET );

And this often does not:

-use Mojo::DOM;
+use Mojo::DOM ();

All of the existing options above are for targeting specific modules by name and not for modifying general behavior.

[oalders]

My problem with use Mojo::DOM; is that I have to know that Mojo::DOM does not, by default, import anything into the current package. use Mojo::DOM (); makes clear that this is not happening, which to me seems to reduce the cognitive load.

[mrsdizzie]

I think the idea is that if you run perlimports you don't need to 'know' anything because you trust perlimports to know it (as it does now) and there is an aesthetic preference for the use statement without () when it doesn't import anything. What you say makes sense, I think the request is just for an option since having the empty () is a bit more subjective as an improvement.

[ThomasLamprecht]

Yeah, sorry for not getting around to post an actual example and thanks to @mrsdizzie for doing so, it's basically what I meant, and I also agree with their arguments.

And I am certainly not arguing for changing the default behavior, I would just be very grateful if an option for such behavior was added.

[oalders]

I took some time to think about this. If someone or some people were to take on the entire lifecycle of this feature, then I think we could do this. That would mean:

• implement the feature and flag it as beta/experimental
• find a competent code reviewer for a first pass
• stick around for bug fixes after release

[ThomasLamprecht]

Thank you for your time evaluating this.

implement the feature and flag it as beta/experimental
stick around for bug fixes after release

Those two I might be able to do. As in, try to whip up a proof-of-concept, then judge on the basis of the changes required and the experience gained on the code base, if my available time should be enough for fleshing that out including tests, docs, ... and also maintaining such a thing in the future.

find a competent code reviewer for a first pass

From the wording I take that this would be (ideally) somebody else than you? Don't get me wrong, that would seem fair enough to me considering that you would not use that feature yourself, I just want to clarify this.

After a quick check it seems to me that one might want to start by looking at the _build_formatted_ppi_statement, i.e.:

App-perlimports/lib/App/perlimports/Include.pm

Lines 573 to 589 in 69318e5

	my $padding = $self->_pad_imports ? q{ } : q{};
	my $template
	= $self->_isa_test_builder_module
	? 'use %s%s import => [ qw(%s%s%s) ];'
	: 'use %s%s qw(%s%s%s);';
	$statement = sprintf(
	$template,
	$self->module_name,
	$maybe_module_version,
	$padding,
	join(
	q{ },
	@{ $self->_imports }
	),
	$padding,
	);

Again, I did not yet check thoroughly, but one might implement this then by passing the new option down to that method and check in there if there are any $self->_imports left, if there are none and the new "suppress qw()" option is enabled then adapt the template string, or well, what gets appended to $statement. I'd appreciate any correction/pointer if that quick evaluation goes rather completely in the wrong direction.

[oalders]

@ThomasLamprecht this all sounds good. I only mention the other code reviewer because that can help speed things along if it's a bigger diff. Having said that, I'm at the Perl Toolchain Summit from Thursday to Sunday of this week, so I'll have plenty of time for code review if you have some bandwidth during that time. Maybe don't worry about finding a reviewer right now.

I believe you are on a good path as far as code changes go.

[ThomasLamprecht]

I sadly had not much leftover time in the last two weeks, but I got around now to implement this. Change wise it's mostly plumbing code, i.e. wiring up the new option, but I might naturally have overlooked something. That said, test coverage in this project seems to be in quite good shape, and they still all run fine on my side.

Would be great if anybody could take a look at the code and option names and meta stuff, but again, definitively no time pressure from my side. And if it helps I can ask a colleague to take a look, I'm just not really certain how helpful that is, as they are not accustomed much to this code base and might have some bias.