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

NoUnusedImportsFixer - Do not remove imports of the same namespace (if configured) #4243

Open
wants to merge 1 commit into
base: 2.13
from

Conversation

Projects
None yet
3 participants
@arnaud-lb
Copy link

arnaud-lb commented Jan 7, 2019

Removing imports of the same namespace is harmful (see #4159).

This PR disables this behaviour by default in the no_unused_imports fix. It also adds a treat_same_namespace_as_unused option (false by default) that allows to restore the old behaviour.

I will submit a fixed to add imports for classes of the same namespace in a separate PR.

@arnaud-lb arnaud-lb force-pushed the arnaud-lb:imports-same-namespace branch 2 times, most recently from 4de7f1f to 3b27220 Jan 7, 2019

@SpacePossum SpacePossum added the feature label Jan 7, 2019

@SpacePossum SpacePossum changed the title Do not remove imports of the same namespace by default NoUnusedImportsFixer - Do not remove imports of the same namespace by default Jan 7, 2019

@SpacePossum SpacePossum changed the title NoUnusedImportsFixer - Do not remove imports of the same namespace by default NoUnusedImportsFixer - Do not remove imports of the same namespace (if configured) Jan 7, 2019

@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Jan 7, 2019

Hi @arnaud-lb and thanks for all your work here!

Am I correct to see that you change the default behavior of the rule? If so it would be a BC break (at least in this project ;) ) so maybe it would be better to add the option here and later on propose to change the default behavior (on the next major) as it would allow for discussion on that topic :)
Secondly, it would be nice to have test for both configuration options. You added a lot of test already (big 👍 ), but please at test for both values for the new config option starting with the most simple ones.

@kubawerlos

This comment has been minimized.

Copy link
Contributor

kubawerlos commented Jan 7, 2019

I will submit a fixed to add imports for classes of the same namespace in a separate PR.

I would downvote such a PR as I believe unused imports are not worth to keep in prospect of future changing namespace of the class.

@arnaud-lb

This comment has been minimized.

Copy link

arnaud-lb commented Jan 8, 2019

@SpacePossum alright, I will change the PR to keep the current behaviour by default :)

The tests I've added are testing both configurations. I will make this more explicit.

@arnaud-lb

This comment has been minimized.

Copy link

arnaud-lb commented Jan 8, 2019

@kubawerlos: YAGNI is one of my favorite acronym principles. When following it, we lean towards thinking that if a hypothetical feature is easy to add later, we should do so. This prevents introducing complexity that may not actually be needed in the future, or worse, that may not match the use-cases when the feature is eventually needed.

However, I don't think that it applies here. We are not keeping imports for a hypothetical need: We are keeping them for an actual, current threat: When some code uses a class of the same namespace, and this class is not explicitly imported, it's easy to accidentally shadow it by importing an other class with the same name from an other namespace.

Our goal as developpers is to make the code easy to read and to change.

I don't really understand the need for removing imports of the same namespace. They make the code clearer / more explicit IMHO. I also don't see how they could hurt in any way (performance wise, they are completely free, in case this is the motivation).

(edit: typo / wording)

@kubawerlos

This comment has been minimized.

Copy link
Contributor

kubawerlos commented Jan 8, 2019

@arnaud-lb

it's easy to accidentally shadow it

What do you mean? Have you seen my example from the issue?

Our goal as developpers is to make the code easy to read and to change.

Easy to read? Definitely. But easy to change? I would'n say it so important to keep unused imports to be prepared for it.

I also don't see how they could hurt in any way.

I look at it from the opposite side - they don't do any good - let's get rid of them.

@arnaud-lb

This comment has been minimized.

Copy link

arnaud-lb commented Jan 9, 2019

it's easy to accidentally shadow it

What do you mean? Have you seen my example from the issue?

I've just answered to your comment in the issue

I look at it from the opposite side - they don't do any good - let's get rid of them.

I've shown that in some cases, they do at least some good :) They also have the merit of making things more explicit, which have some value for readability IMHO.

return new FixerConfigurationResolver([
(new FixerOptionBuilder('treat_same_namespace_as_unused', 'whether to treat imports in the same namespace as unused'))
->setAllowedTypes(['bool'])
->setDefault(false)

This comment has been minimized.

@SpacePossum

SpacePossum Jan 11, 2019

Member

please default to true (and regenerate the readme) so the default behavior of the rule doesn't change on the minor release

@arnaud-lb arnaud-lb force-pushed the arnaud-lb:imports-same-namespace branch from 3b27220 to b84b1b0 Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment