Fix dynamically-set all variable #1659

DeflateAwning · 2023-09-12T16:14:26Z

Fixes issue #1656 by statically setting the __all__ variable.
Note that this could probably be done more preciely. The list was generated using the dynamic generation code currently there, printed, and then inserted back in.

Byron

Thanks a lot!

It looks like the list is not complete though, is this intentional?

Out[19]:
['Actor',
 'AmbiguousObjectName',
 'BadName',
 'BadObject',
 'BadObjectType',
 'BaseIndexEntry',
 'Blob',
 'BlobFilter',
 'BlockingLockFile',
 'CacheError',
 'CheckoutError',
 'CommandError',
 'Commit',
 'Diff',
 'DiffIndex',
 'Diffable',
 'FetchInfo',
 'Git',
 'GitCmdObjectDB',
 'GitCommandError',
 'GitCommandNotFound',
 'GitConfigParser',
 'GitDB',
 'GitError',
 'HEAD',
 'Head',
 'HookExecutionError',
 'IndexEntry',
 'IndexFile',
 'IndexObject',
 'InvalidDBRoot',
 'InvalidGitRepositoryError',
 'List',
 'LockFile',
 'NULL_TREE',
 'NoSuchPathError',
 'ODBError',
 'Object',
 'Optional',
 'ParseError',
 'PathLike',
 'PushInfo',
 'RefLog',
 'RefLogEntry',
 'Reference',
 'Remote',
 'RemoteProgress',
 'RemoteReference',
 'Repo',
 'RepositoryDirtyError',
 'RootModule',
 'RootUpdateProgress',
 'Sequence',
 'StageType',
 'Stats',
 'Submodule',
 'SymbolicReference',
 'TYPE_CHECKING',
 'Tag',
 'TagObject',
 'TagReference',
 'Tree',
 'TreeModifier',
 'Tuple',
 'Union',
 'UnmergedEntriesError',
 'UnsafeOptionError',
 'UnsafeProtocolError',
 'UnsupportedOperation',
 'UpdateProgress',
 'WorkTreeRepositoryUnsupported',
 'remove_password_if_present',
 'rmtree',
 'safe_decode',
 'to_hex_sha']

In [20]: len(git.__all__)
Out[20]: 75

In [21]: len(all)
Out[21]: 67

Could you also sort the list please? This makes it easier to maintain I think. Thank you.

DeflateAwning · 2023-09-14T06:08:16Z

I simply ran the existing code that was there, and then hard-coded it, removing entries that Pyright said were invalid.

Is this new list the complete list that should be used?

Byron · 2023-09-14T06:55:50Z

[..] removing entries that Pyright said were invalid.

I think this explains the mismatch - I created this list based on what's actually there, without looking at a linter. My take on this is that it should be a complete substitution, and no entry can be missing, in order to remain compatible.

Is this new list the complete list that should be used?

Yes, I think so. It would be interesting to know what Pyright is complaining about, but would hope that can be fixed without altering the contents of __all__.

Thanks for bearing with me on this one.

- explicit imports in exc added to avoid linting errors in __init__

DeflateAwning · 2023-10-06T18:19:15Z

Implemented these changed, please merge :)

DeflateAwning · 2023-10-06T18:24:55Z

To fully fix the type hinting issues in all files I've modified here, merge this PR on gitdb, and bump the submodule version

Byron · 2023-10-07T12:22:27Z

It seems the NOQA part doesn't kick in for some reason - maybe this can also be validated locally by use of a venv or tox.

DeflateAwning · 2023-10-10T21:32:08Z

Should finally be good now! You'll probably want to squash and merge though, as I had a time stuggling with the linter (some new things were learned though)

Byron · 2023-10-11T07:06:46Z

Thanks a lot, it seems correct now.

For future reference, please prefer to rebase instead of merging. Unfortunately my tooling isn't quite up to the task to visualize complex commit graphs nicely (or I don't know how to use them).

…opers#1659 When gitpython-developers#1659 was updated to pick up linting configuration changes, it inadvertently undid one of the URL changes made in gitpython-developers#1662, putting the URL in the git.exe module back to the one that redirects to a different BSD license from the one this project uses. Since only that one module was affected, the fix is simple. This only changes the URL back; it doesn't undo any other gitpython-developers#1659 changes.

DeflateAwning · 2023-10-11T15:38:20Z

I'll have to figure that out in GitHub Desktop

…opers#1659 When gitpython-developers#1659 was updated to pick up linting configuration changes, it inadvertently undid one of the URL changes made in gitpython-developers#1662, putting the URL in the git.exc module back to the one that redirects to a different BSD license from the one this project uses. Since only that one module was affected, the fix is simple. This only changes the URL back; it doesn't undo any other gitpython-developers#1659 changes.

Fix small #1662 regression due to #1659

DeflateAwning · 2023-10-17T01:12:24Z

Can you do a release with these changes soon?

Byron · 2023-10-17T06:10:44Z

And here it is: https://github.com/gitpython-developers/GitPython/releases/tag/3.1.38 .

DeflateAwning · 2023-10-17T06:21:58Z

Thank you!

The git.exc module imports exceptions from gitdb.exc to republish them, as well as defining its own (also for use from outside). But because it did not define __all__, the intent for the exceptions it imported was unclear, since names that are introduced by imports and not present in __all__ are not generally considered public, even when __all__ is absent and a "*" import would reimport them. This rectifies that by adding __all__ and listing both imported and newly introduced exceptions explicitly in it. Although this strictly expands which names are public under typical conventions, it strictly contracts which names are imported by a "*" import, because the presence of __all__ suppresses names not listed in it from being imported that way. However, because under typical conventions those other names are not considered public, and they were not even weakly documented as public, this should be okay. (Even though this is not a breaking change, in that code it would break would already technically be broken... if it turns out that it is common to wrongly rely on the availabiliy of those names, then this may need to be revisited and slightly modified.) This brings the readily identified public interface of git.exc in line with what is weakly implied (and intended) by its docstring. This also modifies __init__.py accordingly: The top-level git module has for some time used a "*" import on git.exc, causing the extra names originally meant as implementation details to be included. Because its own __all__ was dynamically generated until c862845, gitpython-developers#1659 also added 8edc53b to retain the formerly present names in __all__. So the change here imports those names from the modules that deliberately provide them, to preserve compatibility.

This makes the git.refresh function unambiguously public. git.refresh was already public in the sense that it was explicitly documented as appropriate to call from code outside GitPython. However, it had not been included in git.__all__. Because __all__ existed but omitted "refresh", git.refresh had appeared non-public to automated tools. This also does some cleanup: - It removes a comment that showed how git.__all__ had been defined dynamically before gitpython-developers#1659, since with the addition of "refresh", git.__all__ no longer contains exactly the same elements as that technique produced (as it examined the module's contents prior to running the def statement that bound the name "refresh"). - With that comment removed, it is no longer necessary to define __all__ below the imports to show what the dynamic techinque had operated on. So this moves it up above them in accordance with PEP-8.

This adds comments to entries in git.__all__ for each of the entries that come from the standard library typing module, noting them as deprecated. These imports were included in __all__ inadvertently due to the way __all__ was dynamically constructed, and placed in __all__ explicitly when __all__ became static in gitpython-developers#1659. They are there for backward compatibility, in case some code relies on them being there. But a module is unlikely to rely intentionally on the git module providing them, since they are not conceptually related to GitPython. `from git import *` should not typically be used, since wildcard imports are not generally recommended, as discussed in PEP-8. But if someone does choose to use it, they would probably benefit less from DeprecationWarning being issued for each of those names than they would usually benefit from DeprecationWarning. This could lead to developers deciding not to enable DeprecationWarning when it may otherwise be useful. For this reason, no attempt is currently made to issue DeprecationWarning when those names are accessed as attributes of the git module.

This makes the git.refresh function unambiguously public. git.refresh was already public in the sense that it was explicitly documented as appropriate to call from code outside GitPython. However, it had not been included in git.__all__. Because __all__ existed but omitted "refresh", git.refresh had appeared non-public to automated tools. This also does some cleanup: - It removes a comment that showed how git.__all__ had been defined dynamically before gitpython-developers#1659, since with the addition of "refresh", git.__all__ no longer contains exactly the same elements as that technique produced (as it examined the module's contents prior to running the def statement that bound the name "refresh"). - With that comment removed, it is no longer necessary to define __all__ below the imports to show what the dynamic techinque had operated on. So this moves it up above them in accordance with PEP-8.

This adds comments to entries in git.__all__ for each of the entries that come from the standard library typing module, noting them as deprecated. These imports were included in __all__ inadvertently due to the way __all__ was dynamically constructed, and placed in __all__ explicitly when __all__ became static in gitpython-developers#1659. They are there for backward compatibility, in case some code relies on them being there. But a module is unlikely to rely intentionally on the git module providing them, since they are not conceptually related to GitPython. `from git import *` should not typically be used, since wildcard imports are not generally recommended, as discussed in PEP-8. But if someone does choose to use it, they would probably benefit less from DeprecationWarning being issued for each of those names than they would usually benefit from DeprecationWarning. This could lead to developers deciding not to enable DeprecationWarning when it may otherwise be useful. For this reason, no attempt is currently made to issue DeprecationWarning when those names are accessed as attributes of the git module.

This makes the git.refresh function unambiguously public. git.refresh was already public in the sense that it was explicitly documented as appropriate to call from code outside GitPython. However, it had not been included in git.__all__. Because __all__ existed but omitted "refresh", git.refresh had appeared non-public to automated tools. This also does some cleanup: - It removes a comment that showed how git.__all__ had been defined dynamically before gitpython-developers#1659, since with the addition of "refresh", git.__all__ no longer contains exactly the same elements as that technique produced (as it examined the module's contents prior to running the def statement that bound the name "refresh"). - With that comment removed, it is no longer necessary to define __all__ below the imports to show what the dynamic techinque had operated on. So this moves it up above them in accordance with PEP-8.

This adds comments to entries in git.__all__ for each of the entries that come from the standard library typing module, noting them as deprecated. These imports were included in __all__ inadvertently due to the way __all__ was dynamically constructed, and placed in __all__ explicitly when __all__ became static in gitpython-developers#1659. They are there for backward compatibility, in case some code relies on them being there. But a module is unlikely to rely intentionally on the git module providing them, since they are not conceptually related to GitPython. `from git import *` should not typically be used, since wildcard imports are not generally recommended, as discussed in PEP-8. But if someone does choose to use it, they would probably benefit less from DeprecationWarning being issued for each of those names than they would usually benefit from DeprecationWarning. This could lead to developers deciding not to enable DeprecationWarning when it may otherwise be useful. For this reason, no attempt is currently made to issue DeprecationWarning when those names are accessed as attributes of the git module.

The submodules being made explicit here are of course Python submodules, not git submodules. The git.objects.submodule Python submodule (which is *about* git submodules) is made explicit here (as are the names imported from that Python submodule's own Python submodules) along with the other Python submodules of git.objects. Unlike some other submodules, but like the top-level git module until gitpython-developers#1659, git.objects already defined __all__ but it was dynamically constructed. As with git.__all__ previously (as noted in gitpython-developers#1859), I have used https://github.com/EliahKagan/deltall here to check that it is safe, sufficient, and probably necessary to replace this dynamically constructed __all__ with a literal list of strings in which all of the original elements are included. See: https://gist.github.com/EliahKagan/e627603717ca5f9cafaf8de9d9f27ad7 Running the modattrs.py script, and diffing against the output from before the current import refactoring began, reveals two changes to module contents as a result of the change here: - git.objects no longer contains `inspect`, which it imported just to build the dynamically constructed __all__. Because this was not itself included in that __all__ or otherwise made public or used out of git.objects, that is okay. This is exactly analogous to the situation in 8197e90, which removed it from the top-level git module after gitpython-developers#1659. - The top-level git module now has has new attributes blob, commit, submodule, and tree, aliasing the corresponding modules. This has happened as a result of them being put in git.objects.__all__ along with various names imported from them. (As noted in some prior commits, there are tradeoffs associated with doing this, and it may be that such elements of __all__ should be removed, here and elsewhere.)

Fix dynamically-set __all__ variable

c862845

Byron requested changes Sep 13, 2023

View reviewed changes

EliahKagan mentioned this pull request Sep 21, 2023

Remove @NoEffect annotations #1677

Merged

EliahKagan mentioned this pull request Oct 3, 2023

Clarify Git.execute and Popen arguments #1688

Merged

Clean up __all__ in main, and explicit imports in exc

8edc53b

- explicit imports in exc added to avoid linting errors in __init__

Add @UnusedImport to recent changes

9ac6bb0

DeflateAwning added 5 commits October 10, 2023 15:03

Fix unused import linting

ebd9ce5

Typo

b1478bb

Merge branch 'main' into main

fbac514

black lint

a4eb2b3

Merge branch 'main' of https://github.com/DeflateAwning/GitPython

87cc325

Byron merged commit 87cc325 into gitpython-developers:main Oct 11, 2023
8 checks passed

EliahKagan mentioned this pull request Oct 11, 2023

Fix small #1662 regression due to #1659 #1701

Merged

Byron added a commit that referenced this pull request Oct 12, 2023

Merge pull request #1701 from EliahKagan/license-ambiguity-regression

1e7d885

Fix small #1662 regression due to #1659

This was referenced Oct 12, 2023

Update CI, in line with gitdb gitpython-developers/smmap#53

Merged

Have Dependabot offer submodule updates #1702

Merged

DeflateAwning mentioned this pull request Oct 17, 2023

Fix warning in Pyright ("Repo" not exported from module "git") #1656

Closed

This was referenced Oct 18, 2023

Have init script clone submodules unconditionally #1715

Merged

3.1.38 meant to fix #1656 but there is no supporting gitdb release #1716

Closed

We appear to import the submodule version of gitdb (but never do) #1717

Closed

This was referenced Oct 20, 2023

The public interface of git.exc is ambiguous #1718

Closed

Add __all__ in git.exc #1719

Merged

EliahKagan mentioned this pull request Jan 19, 2024

Override bash executable, defaulting to Git for Windows git bash over WSL bash #1791

Open

EliahKagan mentioned this pull request Feb 20, 2024

Overbroad xfail marks will eventually make CI fail #1728

Closed

This was referenced Mar 11, 2024

Improve static typing and docstrings related to git object types #1859

Merged

Top-level refresh function not listed in __all__ #1875

Closed

EliahKagan mentioned this pull request Mar 19, 2024

Replace all wildcard imports with explicit imports #1880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dynamically-set all variable #1659

Fix dynamically-set all variable #1659

DeflateAwning commented Sep 12, 2023 •

edited

Loading

Byron left a comment

DeflateAwning commented Sep 14, 2023

Byron commented Sep 14, 2023

DeflateAwning commented Oct 6, 2023

DeflateAwning commented Oct 6, 2023 •

edited

Loading

Byron commented Oct 7, 2023

DeflateAwning commented Oct 10, 2023

Byron commented Oct 11, 2023

DeflateAwning commented Oct 11, 2023

DeflateAwning commented Oct 17, 2023

Byron commented Oct 17, 2023

DeflateAwning commented Oct 17, 2023

Fix dynamically-set __all__ variable #1659

Fix dynamically-set __all__ variable #1659

Conversation

DeflateAwning commented Sep 12, 2023 • edited Loading

Byron left a comment

Choose a reason for hiding this comment

DeflateAwning commented Sep 14, 2023

Byron commented Sep 14, 2023

DeflateAwning commented Oct 6, 2023

DeflateAwning commented Oct 6, 2023 • edited Loading

Byron commented Oct 7, 2023

DeflateAwning commented Oct 10, 2023

Byron commented Oct 11, 2023

DeflateAwning commented Oct 11, 2023

DeflateAwning commented Oct 17, 2023

Byron commented Oct 17, 2023

DeflateAwning commented Oct 17, 2023

Fix dynamically-set all variable #1659

Fix dynamically-set all variable #1659

DeflateAwning commented Sep 12, 2023 •

edited

Loading

DeflateAwning commented Oct 6, 2023 •

edited

Loading