Overhaul noqa directives

- Remove some noqa directives that are not needed at all anymore.
  But there may still be some others I did not find.

- Fix the code and remove the noqa direcive in places where the
  change is very simple and does not affect the interface (e.g.,
  what could be taken to be public or not, what symbols are
  available and what they refer to, etc.), the change makes the
  code more idiomatic or clearer, and it fixes the flake8 complaint
  completely.

- Turn file-level noqa directives into statement-level ones. A noqa
  directive at file level, even if it lists one or more specific
  error/warning numbers, actually suppresses flake8 for the entire
  file. So the main benefit here is to enable other warnings. But
  this also makes it clear what is producing the ones we suppress,
  which makes it easier to know what is intentional and how it may
  be reasonable to change it in the future.

- Move noqa directives at the end of multi-line imports to the top
  line, where flake8 honors them, and eliminate redundant
  directives that were added to work around their ineffectiveness.
  (This is not really separate from the above point, since this is
  often what allowed file-level directives to be removed.)

- Specify the errors/warnings each "noqa" should suppress. That is,
  this turns every bare "noqa", aside from those eliminated as
  described above, into a "noqa" for one or more specific
  errors/warnings. This makes the intent clearer and avoids
  oversuppression.

- Add missing ":" between "noqa" and error/warning numbers. When
  these were absent, the intention was to suppress only the
  specifically listed errors/warnings, but the effect was that the
  listed numbers were ignored and *everything* was suppressed. In a
  couple of cases it was necessary to change the listed numbers to
  correct the list to what actually needed to be suppressed.

- Write every "noqa" in lower-case (not "NOQA"). There did not
  appear to be a systematic reason for the different casing, and
  having them all cased the same makes it easier to avoid mistakes
  when grepping for them.

- Where an import is unused and does not appear intended to be
  accesed from other modules, and is present only to support code
  in the same module that is commented out, but whose removal
  should be considered separately, comment out the statement or
  part of the statement that imports it as well, rather than
  writing a "noqa" for the unused import. This applies to only one
  file, git/objects/util.py.

Author: EliahKagan

git/__init__.py

@@ -3,28 +3,27 @@
 # This module is part of GitPython and is released under the
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
-# flake8: noqa
 # @PydevCodeAnalysisIgnore
 
-from git.exc import *  # @NoMove @IgnorePep8
-from typing import List, Optional, Sequence, Tuple, Union, TYPE_CHECKING
-from git.types import PathLike
-
 __version__ = "git"
 
+from typing import List, Optional, Sequence, Tuple, Union, TYPE_CHECKING
+
 from gitdb.util import to_hex_sha
+from git.exc import *  # noqa: F403  # @NoMove @IgnorePep8
+from git.types import PathLike
 
 try:
     from git.compat import safe_decode  # @NoMove @IgnorePep8
     from git.config import GitConfigParser  # @NoMove @IgnorePep8
-    from git.objects import *  # @NoMove @IgnorePep8
-    from git.refs import *  # @NoMove @IgnorePep8
-    from git.diff import *  # @NoMove @IgnorePep8
-    from git.db import *  # @NoMove @IgnorePep8
+    from git.objects import *  # noqa: F403  # @NoMove @IgnorePep8
+    from git.refs import *  # noqa: F403  # @NoMove @IgnorePep8
+    from git.diff import *  # noqa: F403  # @NoMove @IgnorePep8
+    from git.db import *  # noqa: F403  # @NoMove @IgnorePep8
     from git.cmd import Git  # @NoMove @IgnorePep8
     from git.repo import Repo  # @NoMove @IgnorePep8
-    from git.remote import *  # @NoMove @IgnorePep8
-    from git.index import *  # @NoMove @IgnorePep8
+    from git.remote import *  # noqa: F403  # @NoMove @IgnorePep8
+    from git.index import *  # noqa: F403  # @NoMove @IgnorePep8
     from git.util import (  # @NoMove @IgnorePep8
         LockFile,
         BlockingLockFile,
@@ -33,12 +32,12 @@
         remove_password_if_present,
         rmtree,
     )
-except GitError as _exc:
+except GitError as _exc:  # noqa: F405
     raise ImportError("%s: %s" % (_exc.__class__.__name__, _exc)) from _exc
 
 # __all__ must be statically defined by py.typed support
 # __all__ = [name for name, obj in locals().items() if not (name.startswith("_") or inspect.ismodule(obj))]
-__all__ = [
+__all__ = [  # noqa: F405
     "Actor",
     "AmbiguousObjectName",
     "BadName",
@@ -127,7 +126,7 @@ def refresh(path: Optional[PathLike] = None) -> None:
 
     if not Git.refresh(path=path):
         return
-    if not FetchInfo.refresh():
+    if not FetchInfo.refresh():  # noqa: F405
         return  # type: ignore [unreachable]
 
     GIT_OK = True

git/cmd.py

@@ -965,7 +965,7 @@ def execute(
             # Only search PATH, not CWD. This must be in the *caller* environment. The "1" can be any value.
             maybe_patch_caller_env = patch_env("NoDefaultCurrentDirectoryInExePath", "1")
         else:
-            cmd_not_found_exception = FileNotFoundError  # NOQA # exists, flake8 unknown @UndefinedVariable
+            cmd_not_found_exception = FileNotFoundError
             maybe_patch_caller_env = contextlib.nullcontext()
         # END handle
 

git/compat.py

@@ -5,20 +5,15 @@
 
 """Utilities to help provide compatibility with Python 3."""
 
-# flake8: noqa
-
 import locale
 import os
 import sys
 
-from gitdb.utils.encoding import (
-    force_bytes,  # @UnusedImport
-    force_text,  # @UnusedImport
-)
+from gitdb.utils.encoding import force_bytes, force_text  # noqa: F401  # @UnusedImport
 
 # typing --------------------------------------------------------------------
 
-from typing import (
+from typing import (  # noqa: F401
     Any,
     AnyStr,
     Dict,

git/index/__init__.py

@@ -3,7 +3,5 @@
 
 """Initialize the index package."""
 
-# flake8: noqa
-
-from .base import *
-from .typ import *
+from .base import *  # noqa: F401 F403
+from .typ import *  # noqa: F401 F403

git/objects/__init__.py

@@ -3,23 +3,21 @@
 
 """Import all submodules' main classes into the package space."""
 
-# flake8: noqa
-
 import inspect
 
-from .base import *
-from .blob import *
-from .commit import *
+from .base import *  # noqa: F403
+from .blob import *  # noqa: F403
+from .commit import *  # noqa: F403
 from .submodule import util as smutil
-from .submodule.base import *
-from .submodule.root import *
-from .tag import *
-from .tree import *
+from .submodule.base import *  # noqa: F403
+from .submodule.root import *  # noqa: F403
+from .tag import *  # noqa: F403
+from .tree import *  # noqa: F403
 
 # Fix import dependency - add IndexObject to the util module, so that it can be
 # imported by the submodule.base.
-smutil.IndexObject = IndexObject  # type: ignore[attr-defined]
-smutil.Object = Object  # type: ignore[attr-defined]
+smutil.IndexObject = IndexObject  # type: ignore[attr-defined]  # noqa: F405
+smutil.Object = Object  # type: ignore[attr-defined]  # noqa: F405
 del smutil
 
 # Must come after submodule was made available.

git/objects/util.py

@@ -5,31 +5,27 @@
 
 """General utility functions."""
 
-# flake8: noqa F401
-
-
 from abc import ABC, abstractmethod
-import warnings
-from git.util import IterableList, IterableObj, Actor
-
-import re
+import calendar
 from collections import deque
-
+from datetime import datetime, timedelta, tzinfo
 from string import digits
+import re
 import time
-import calendar
-from datetime import datetime, timedelta, tzinfo
+import warnings
+
+from git.util import IterableList, IterableObj, Actor
 
 # typing ------------------------------------------------------------
 from typing import (
     Any,
     Callable,
     Deque,
     Iterator,
-    Generic,
+    # Generic,
     NamedTuple,
     overload,
-    Sequence,  # NOQA: F401
+    Sequence,
     TYPE_CHECKING,
     Tuple,
     Type,
@@ -38,7 +34,7 @@
     cast,
 )
 
-from git.types import Has_id_attribute, Literal, _T  # NOQA: F401
+from git.types import Has_id_attribute, Literal  # , _T
 
 if TYPE_CHECKING:
     from io import BytesIO, StringIO

git/refs/__init__.py

@@ -1,13 +1,12 @@
 # This module is part of GitPython and is released under the
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
-# flake8: noqa
 # Import all modules in order, fix the names they require.
 
-from .symbolic import *
-from .reference import *
-from .head import *
-from .tag import *
-from .remote import *
+from .symbolic import *  # noqa: F401 F403
+from .reference import *  # noqa: F401 F403
+from .head import *  # noqa: F401 F403
+from .tag import *  # noqa: F401 F403
+from .remote import *  # noqa: F401 F403
 
-from .log import *
+from .log import *  # noqa: F401 F403

git/refs/log.py

@@ -31,9 +31,9 @@
 from git.types import PathLike
 
 if TYPE_CHECKING:
-    from git.refs import SymbolicReference
     from io import BytesIO
-    from git.config import GitConfigParser, SectionConstraint  # NOQA
+    from git.refs import SymbolicReference
+    from git.config import GitConfigParser, SectionConstraint
 
 # ------------------------------------------------------------------------------
 

git/refs/reference.py

@@ -10,8 +10,8 @@
 
 # typing ------------------------------------------------------------------
 
-from typing import Any, Callable, Iterator, Type, Union, TYPE_CHECKING  # NOQA
-from git.types import Commit_ish, PathLike, _T  # NOQA
+from typing import Any, Callable, Iterator, Type, Union, TYPE_CHECKING
+from git.types import Commit_ish, PathLike, _T
 
 if TYPE_CHECKING:
     from git.repo import Repo

git/refs/symbolic.py

@@ -1,7 +1,6 @@
 # This module is part of GitPython and is released under the
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
-from git.types import PathLike
 import os
 
 from git.compat import defenc
@@ -31,8 +30,8 @@
     Union,
     TYPE_CHECKING,
     cast,
-)  # NOQA
-from git.types import Commit_ish, PathLike  # NOQA
+)
+from git.types import Commit_ish, PathLike
 
 if TYPE_CHECKING:
     from git.repo import Repo

git/repo/__init__.py

@@ -3,6 +3,4 @@
 
 """Initialize the Repo package."""
 
-# flake8: noqa
-
-from .base import Repo as Repo
+from .base import Repo as Repo  # noqa: F401

git/types.py

@@ -1,11 +1,9 @@
 # This module is part of GitPython and is released under the
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
-# flake8: noqa
-
 import os
 import sys
-from typing import (
+from typing import (  # noqa: F401
     Dict,
     NoReturn,
     Sequence as Sequence,
@@ -16,24 +14,24 @@
     Callable,
     TYPE_CHECKING,
     TypeVar,
-)  # noqa: F401
+)
 
 if sys.version_info >= (3, 8):
-    from typing import (
+    from typing import (  # noqa: F401
         Literal,
         TypedDict,
         Protocol,
         SupportsIndex as SupportsIndex,
         runtime_checkable,
-    )  # noqa: F401
+    )
 else:
-    from typing_extensions import (
+    from typing_extensions import (  # noqa: F401
         Literal,
         SupportsIndex as SupportsIndex,
         TypedDict,
         Protocol,
         runtime_checkable,
-    )  # noqa: F401
+    )
 
 # if sys.version_info >= (3, 10):
 #     from typing import TypeGuard  # noqa: F401

git/util.py

@@ -62,12 +62,9 @@
     Has_id_attribute,
 )
 
-T_IterableObj = TypeVar("T_IterableObj", bound=Union["IterableObj", "Has_id_attribute"], covariant=True)
-# So IterableList[Head] is subtype of IterableList[IterableObj]
-
 # ---------------------------------------------------------------------
 
-from gitdb.util import (  # NOQA @IgnorePep8
+from gitdb.util import (  # noqa: F401  # @IgnorePep8
     make_sha,
     LockedFD,  # @UnusedImport
     file_contents_ro,  # @UnusedImport
@@ -79,6 +76,9 @@
     hex_to_bin,  # @UnusedImport
 )
 
+T_IterableObj = TypeVar("T_IterableObj", bound=Union["IterableObj", "Has_id_attribute"], covariant=True)
+# So IterableList[Head] is subtype of IterableList[IterableObj].
+
 # NOTE:  Some of the unused imports might be used/imported by others.
 # Handle once test-cases are back up and running.
 # Most of these are unused here, but are for use by git-python modules so these

test/lib/__init__.py

@@ -3,8 +3,8 @@
 # This module is part of GitPython and is released under the
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
-# flake8: noqa
 import inspect
-from .helper import *
+
+from .helper import *  # noqa: F401 F403
 
 __all__ = [name for name, obj in locals().items() if not (name.startswith("_") or inspect.ismodule(obj))]

test/lib/helper.py

@@ -145,7 +145,7 @@ def repo_creator(self):
             os.chdir(rw_repo.working_dir)
             try:
                 return func(self, rw_repo)
-            except:  # noqa E722
+            except:  # noqa: E722 B001
                 log.info("Keeping repo after failure: %s", repo_dir)
                 repo_dir = None
                 raise
@@ -305,7 +305,7 @@ def remote_repo_creator(self):
                     with cwd(rw_repo.working_dir):
                         try:
                             return func(self, rw_repo, rw_daemon_repo)
-                        except:  # noqa E722
+                        except:  # noqa: E722 B001
                             log.info(
                                 "Keeping repos after failure: \n  rw_repo_dir: %s \n  rw_daemon_repo_dir: %s",
                                 rw_repo_dir,

test/test_commit.py

@@ -475,7 +475,7 @@ def test_datetimes(self):
             commit.authored_datetime,
             datetime(2009, 10, 8, 18, 17, 5, tzinfo=tzoffset(-7200)),
             commit.authored_datetime,
-        )  # noqa
+        )
         self.assertEqual(
             commit.authored_datetime,
             datetime(2009, 10, 8, 16, 17, 5, tzinfo=utc),

test/test_config.py

@@ -125,7 +125,7 @@ def test_multi_line_config(self):
             ev = "ruby -e '\n"
             ev += "		system %(git), %(merge-file), %(--marker-size=%L), %(%A), %(%O), %(%B)\n"
             ev += "		b = File.read(%(%A))\n"
-            ev += "		b.sub!(/^<+ .*\\nActiveRecord::Schema\\.define.:version => (\\d+). do\\n=+\\nActiveRecord::Schema\\."  # noqa E501
+            ev += "		b.sub!(/^<+ .*\\nActiveRecord::Schema\\.define.:version => (\\d+). do\\n=+\\nActiveRecord::Schema\\."  # noqa: E501
             ev += "define.:version => (\\d+). do\\n>+ .*/) do\n"
             ev += "		  %(ActiveRecord::Schema.define(:version => #{[$1, $2].max}) do)\n"
             ev += "		end\n"

test/test_docs.py

@@ -25,7 +25,7 @@ def tearDown(self):
     #
     # @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
     #         "FIXME: helper.wrapper fails with: PermissionError: [WinError 5] Access is denied: "
-    #         "'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\test_work_tree_unsupportedryfa60di\\master_repo\\.git\\objects\\pack\\pack-bc9e0787aef9f69e1591ef38ea0a6f566ec66fe3.idx")  # noqa E501
+    #         "'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\test_work_tree_unsupportedryfa60di\\master_repo\\.git\\objects\\pack\\pack-bc9e0787aef9f69e1591ef38ea0a6f566ec66fe3.idx")  # noqa: E501
     @with_rw_directory
     def test_init_repo_object(self, rw_dir):
         # [1-test_init_repo_object]