[AddImportsVisitor] Docstring Check Only for the Top Element of the Body

[sagarbadiyani]

Summary

Refer #343 (comment)
Instead of checking for docstring for all the elements of the body, this PR only keeps the check for the 0th element since our objective is to only honor the module docstrings.

The reason I am asking for this, I found a case in one of the repositories where the docstrings were written outside the function like how we have in Java, obviously, this is unconventional and unpythonic, but my only concern is why are we checking for docstrings which are not module docstrings?

Consider a case

from __future__ import annotations
from __future__ import division

import abc
from xyz import def

"""Some docstring"""
def some_func():
    pass

Let's say we are trying to add an import from typing import List, the final code after applying the add imports tranformation will look like

from __future__ import annotations
from typing import List
from __future__ import division

import abc
from xyz import def

"""Some docstring"""
def some_func():
    pass

This is because the docstring is a part of the body of the module and when the for loop reached there, it set the import_add_location to 1, which should not have been the case
And this will break the code because all the from __future__ imports must occur at the beginning of the file

Test Plan

Added a Testcase.

[sagarbadiyani]

Hey @zsol can you please take a look?

[sagarbadiyani]

@jimmylai @carljm would be great if you could take a look at this one :)

[carljm]

This looks like a correct fix to me. Thanks for adding a test!

One test-naming nit, and it looks like CI is not fully happy.

[carljm]

nit: A "docstring" in the wrong location is not really a docstring at all, technically speaking. It's just a random string expression in a statement by itself, which is thrown away at runtime. So I would prefer to rename this test to test_import_in_module_with_standalone_string_not_a_docstring.

[sagarbadiyani]

Agreed, thank you

[codecov-commenter]

Codecov Report

Base: 94.79% // Head: 94.79% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (4b2afd4) compared to base (f668e88).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #841   +/-   ##
=======================================
  Coverage   94.79%   94.79%           
=======================================
  Files         249      249           
  Lines       25827    25831    +4     
=======================================
+ Hits        24483    24487    +4     
  Misses       1344     1344           

Impacted Files	Coverage Δ
libcst/codemod/visitors/_add_imports.py	95.45% <100.00%> (ø)
libcst/codemod/visitors/tests/test_add_imports.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[sagarbadiyani]

Sorry for bothering again @carljm but any pointers on how can I fix the failing CI checks?
Running python setup.py test locally did not help

[carljm]

any pointers on how can I fix the failing CI checks?

It looks to me like an infra failure unrelated to this PR.

[carljm]

This looks good to me. I think the failing checks are an infra problem that isn't related to this diff.

I haven't been an active maintainer of LibCST for quite a while so I'd prefer to wait a bit and see if someone else can verify that and merge this. If nobody weighs in in the next few days I can probably merge, I think the fix is clearly correct.

[sagarbadiyani]

Great, thank you very much!

[zsol]

Thanks for this @sagarbadiyani!