How about removing unused imports?

[Joachim Durchholz]

Hey all,

I'd like to work on cutting down the number of unused imports; a sample
pull request is on https://github.com/sympy/sympy/pull/8761 .

The questions I'd like to discuss is why this should be done, and how
this should be done.

On the "why":
01) It's needless clutter in the code. People can't look into the import
list to determine the true intermodule dependencies. (+1)
02) Over time, unneeded imports tend to create import cycles. (+1)
03) More imports make refactoring harder that are affected by module
dependencies. Unneeded imports make such tasks gratuitiously harder.
This affects things like eliminating C (actually that's the reason why
I'm looking at imports at all). (+1)
04) I do not know whether it is possible to have an automated test for
unused imports. It may restrict our ability to import stuff depending on
runtime decisions, but I don't really know whether these restrictions
will hurt or won't matter. I hope to find that out by eliminating
imports one by one and seeing whether the test suite fails. (+0)
05) Do we want to eliminate all unused imports, as a code quality check?
It would be desirable to prevent inadvertent import cycles, but we may
not want the restrictions that come with it. (Depends on 04)

On the "how" (assuming the "why" resolves to "yes we want that work done"):
10) Should the modification be rolled into one PR, or do people prefer
smaller PRs? What's the ideal size?

Some general remarks:
20) PyDev is telling me about ~1200 unused imports in the sympy module.
150 are in sympy.mpmath, so they're irrelevant; I don't know how many of
these are false positives.
21) For stuff outside of sympy (i.e. bin, build etc): I'm currently
leaving that alone because I don't know how to test whether a
modification there breaks anything.

Any feedback appreciated.

Regards,
Jo

[Sergey Kirpichev]

On Wednesday, January 7, 2015 1:15:48 PM UTC+3, Joachim Durchholz wrote:

Hey all,

I'd like to work on cutting down the number of unused imports; a sample
pull request is on https://github.com/sympy/sympy/pull/8761 .

If we can first add a test for this in the code quality module - I'm +1.

[Joachim Durchholz]

Am 07.01.2015 um 14:51 schrieb Sergey Kirpichev:
> If we can first add a test for this in the code quality module - I'm +1.

I'm not sure that that is possible.

Also, I'd want to run the test just locally, and push the fixes, before
adding the tests to the code quality module.
With any other order, the next person to git pull will get a nasty
surprise: all their bin/test runs will fail with those newly tested-for
errors, so they can't rely on that "I can make the PR as soon as all
tests pass" rule anymore.

However, I'd also like to know if such fixes are acceptable if it turns
out we can't make it an automated test. It's still an improvement, even
if it won't be long-lived - I could use that at leasts for my upcoming
work on getting rid of the C class.

[Sergey Kirpichev]

On Wed, Jan 7, 2015 at 6:31 PM, Joachim Durchholz <j...@durchholz.org> wrote:
> I'm not sure that that is possible.

That's bad news.

> With any other order, the next person to git pull will get a nasty surprise:
> all their bin/test runs will fail with those newly tested-for errors, so
> they can't rely on that "I can make the PR as soon as all tests pass" rule
> anymore.

You can break other people PR's in a zillons of ways. For example,
their pr can rely on removed import.

To be sure that tests pass - they will have to merge with master
first. That's the rule.

> However, I'd also like to know if such fixes are acceptable if it turns out
> we can't make it an automated test.

Sure, they are. But at least, please try to minimize number of such PR's.
(I.e., do this for all modules once)

[Joachim Durchholz]

Am 07.01.2015 um 17:08 schrieb Sergey Kirpichev:
> On Wed, Jan 7, 2015 at 6:31 PM, Joachim Durchholz <j...@durchholz.org> wrote:
>> I'm not sure that that is possible.
>
> That's bad news.
>
>> With any other order, the next person to git pull will get a nasty surprise:
>> all their bin/test runs will fail with those newly tested-for errors, so
>> they can't rely on that "I can make the PR as soon as all tests pass" rule
>> anymore.
>
> You can break other people PR's in a zillons of ways. For example,
> their pr can rely on removed import.

Yes, but then they have to merge anyway.
If I add a new test that goes over all source files, they will have not
only their changes flagged but also pre-existing code.

>> However, I'd also like to know if such fixes are acceptable if it turns out
>> we can't make it an automated test.
>
> Sure, they are. But at least, please try to minimize number of such PR's.
> (I.e., do this for all modules once)

That's going to be a huge PR that touches all of SymPy.
Remember: 1,000+ reports of unnecessary imports (including false
positives, but I bet it's not going to be much less than 900 changed lines).

That's going to be hard on the eyeball checking.
Of course people might find it worth it anyway. It's a merely technical
change, so as long as the tests pass, it *should* be fine.

It's also going to be more work to keep the PR mergeable as other PRs go in.
Though... I could do that *shrug*.

Let me see what others say. If it's okay by at least one additional
contributor, I'll go ahead and do a big PR for that.

[Aaron Meurer]

Testing for unused imports is possible (pyflakes does it), but I am -1 to adding an automated test for it. Unused imports are clutter, and PRs removing them are welcome, but they are not so bad that they need to be always clean at every point in time. Also there can be false positives, because a function can be imported just so that it can be recursively imported from that same module. At best, you have to ignore __init__.py files. 

Aaron Meurer

--
You received this message because you are subscribed to the Google Groups "sympy" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sympy+unsubscribe@googlegroups.com.
To post to this group, send email to sy...@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.
To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/54AD5FAB.2090604%40durchholz.org.

For more options, visit https://groups.google.com/d/optout.

[Joachim Durchholz]

Am 08.01.2015 um 02:29 schrieb Aaron Meurer:
> Testing for unused imports is possible (pyflakes does it), but I am -1 to
> adding an automated test for it. Unused imports are clutter, and PRs
> removing them are welcome,

Okay, working on these then.

> but they are not so bad that they need to be
> always clean at every point in time.

Understood.

> Also there can be false positives,
> because a function can be imported just so that it can be recursively
> imported from that same module. At best, you have to ignore __init__.py
> files.

That's the stuff that gets re-exported up to the sympy module itself, so
that it is available to the SymPy user, right?

Question is: How do I test that I didn't inadvertently remove an import
for that? I'm not going to see a test failure for that, I think.

[Björn Dahlgren]

 > Also there can be false positives,
> because a function can be imported just so that it can be recursively
> imported from that same module. At best, you have to ignore __init__.py
> files.

That's the stuff that gets re-exported up to the sympy module itself, so
that it is available to the SymPy user, right?

Just wanted to mention that that there is pytest-flakes and pytest-pep8 on PyPI if
SymPy ever switches to depending on pytest externally. Pyflakes lack a way
to indicate that an unused import is intentional (for e.g. try/except ImportError).
For pytest-flakes you can add ignore patterns to e.g. setup.cfg.

There is also flake8 which enables you to via comments to ignore warnings
on a per line basis. And there are extensions to flake8 (e.g. https://github.com/openstack-dev/hacking).
-But, IMHO I think it is very easy to go a bit overboard with these things.
(starting a new project with automatic tests from day 1 is another story..)

[Aaron Meurer]

On Thu, Jan 8, 2015 at 5:15 PM, Björn Dahlgren <bjo...@gmail.com> wrote:

 > Also there can be false positives,
> because a function can be imported just so that it can be recursively
> imported from that same module. At best, you have to ignore __init__.py
> files.

That's the stuff that gets re-exported up to the sympy module itself, so
that it is available to the SymPy user, right?

Just wanted to mention that that there is pytest-flakes and pytest-pep8 on PyPI if
SymPy ever switches to depending on pytest externally. Pyflakes lack a way
to indicate that an unused import is intentional (for e.g. try/except ImportError).
For pytest-flakes you can add ignore patterns to e.g. setup.cfg.

The pyflakes authors want you to use __all__ in __init__.py. You can also silence unused variable or unused import warnings by putting the variable on a line by itself, like

import stuff

stuff

will not give any errors.

Aaron Meurer

 

There is also flake8 which enables you to via comments to ignore warnings
on a per line basis. And there are extensions to flake8 (e.g. https://github.com/openstack-dev/hacking).
-But, IMHO I think it is very easy to go a bit overboard with these things.
(starting a new project with automatic tests from day 1 is another story..)

--

You received this message because you are subscribed to the Google Groups "sympy" group.

To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.

To post to this group, send email to sy...@googlegroups.com.
Visit this group at http://groups.google.com/group/sympy.

To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/b7259156-865b-442f-b9ac-1dee02c03d4a%40googlegroups.com.

[Joachim Durchholz]

Am 09.01.2015 um 00:15 schrieb Björn Dahlgren:
>
> There is also flake8 which enables you to via comments to ignore warnings
> on a per line basis.

I'd actually prefer that.
Such a comment tells the reviewer that "hey, this unused import is
intentional, please take a closer look before removing it".