Closed jorio closed 6 months ago
This is great @jorio , we just need to get the names right because we won't be able to change them later.
My first reflex would be to do a literal translation from the libgit2 constants, as you have done in some cases, like:
GIT_APPLY_LOCATION_WORKDIR -> ApplyLocation.WORKDIR
But for others this is not so obvious. For some you have chosen to add the suffix Flags
, like:
GIT_ATTR_CHECK_FILE_THEN_INDEX -> AttrCheckFlags.FILE_THEN_INDEX
An alternative would be to namespace the enums and flags, for example:
from pygit2 import flags
flags.AttrCheck
Instead of:
from pygit2 import AttrCheckFlags
AttrCheckFlags
I think this approach would have the following benefits:
pygit2.*
What do you think?
For reference, if I have not made errors, the proposed name changes in this PR could be summarized like...
IntEnum:
GIT_APPLY_LOCATION_WORKDIR ApplyLocation.WORKDIR
GIT_CONFIG_LEVEL_PROGRAMDATA ConfigLevel.PROGRAMDATA
GIT_DELTA_UNMODIFIED DeltaStatus.UNMODIFIED
GIT_DESCRIBE_DEFAULT DescribeStrategy.DEFAULT
GIT_FETCH_PRUNE_UNSPECIFIED FetchPruneMode.UNSPECIFIED
GIT_FILTER_TO_WORKTREE FilterMode.TO_WORKTREE
GIT_OPT_GET_MWINDOW_SIZE LibraryOption.GET_MWINDOW_SIZE
GIT_OBJECT_ANY GitObjectType.ANY
GIT_REFERENCES_ALL ReferenceFilter.ALL
GIT_REPOSITORY_INIT_SHARED_UMASK RepositoryInitMode.SHARED_UMASK
GIT_REPOSITORY_STATE_NONE RepositoryState.NONE
GIT_RESET_SOFT ResetType.SOFT
GIT_STASH_APPLY_PROGRESS_NONE StashApplyProgress.NONE
GIT_SUBMODULE_IGNORE_UNSPECIFIED SubmoduleIgnore.UNSPECIFIED
IntFlag:
GIT_ATTR_CHECK_FILE_THEN_INDEX AttrCheckFlags.FILE_THEN_INDEX
GIT_BLAME_NORMAL BlameFlags.NORMAL
GIT_BLOB_FILTER_CHECK_FOR_BINARY BlobFilterFlags.CHECK_FOR_BINARY
GIT_BRANCH_LOCAL BranchType.LOCAL
GIT_CHECKOUT_NOTIFY_NONE CheckoutNotifyFlags.NONE
GIT_CHECKOUT_NONE CheckoutStrategy.NONE
GIT_CREDENTIAL_USERPASS_PLAINTEXT CredentialType.USERPASS_PLAINTEXT
GIT_DIFF_FLAG_BINARY DeltaFlags.BINARY
GIT_DIFF_FIND_BY_CONFIG DiffFindFlags.FIND_BY_CONFIG
GIT_DIFF_NORMAL DiffFlags.NORMAL
GIT_DIFF_STATS_NONE DiffStatsFlags.NONE
GIT_FEATURE_THREADS FeatureFlags.THREADS
GIT_FILEMODE_UNREADABLE FileMode.UNREADABLE
GIT_FILTER_DEFAULT FilterFlags.DEFAULT
GIT_MERGE_ANALYSIS_NONE MergeAnalysis.NONE
GIT_MERGE_PREFERENCE_NONE MergePreference.NONE
GIT_REF_INVALID ReferenceType.INVALID
GIT_REPOSITORY_INIT_BARE RepositoryInitFlags.BARE
GIT_REPOSITORY_OPEN_NO_SEARCH RepositoryOpenFlags.NO_SEARCH
GIT_REVSPEC_SINGLE RevSpecFlags.SINGLE
GIT_STATUS_CURRENT StatusFlags.CURRENT
GIT_SUBMODULE_STATUS_IN_HEAD SubmoduleStatus.IN_HEAD
GIT_SORT_NONE WalkerSortFlags.NONE
Let's agree on the names first.
Thank you for your time reviewing this!
For context, most class names are actually based on the C typedef name, because the names of the actual values within an enum can be messy. It's difficult to make perfect conversions because there's a lot of overlapping prefixes across different enums.
As for your suggestions:
pygit2.enums
is fine with meenums
and flags
might create some friction for users. In 6 months I'm certainly going to go "uh, is FileMode in enums or flags again?" :)I also believe that users should be free to import enum classes directly (from pygit2.enums import Whatever
) without dealing with name clashes. If you agree with this point, then we should avoid simplifications like BranchType -> Branch
, DiffFlags -> Diff
, or ReferenceType -> Reference
.
Here's a proposal for revised names. I attempted to simplify them a bit while still making sure they're distinct from existing classes in pygit2.
C enum/typedef | pygit2? | Passed to/Returned by |
---|---|---|
GIT_APPLY_LOCATION_WORKDIR git_apply_location_t |
ApplyLocation | Repository.apply(...) Repository.applies(...) |
GIT_ATTR_CHECK_FILE_THEN_INDEX (no typedef) |
AttrCheck | Repository.get_attr(flags=...) |
GIT_BLAME_NORMAL git_blame_flag_t |
BlameFlag? ('Blame' would clash) |
Repository.blame(flags=...) |
GIT_BLOB_FILTER_CHECK_FOR_BINARY git_blob_filter_flag_t |
BlobFilter | BlobIO(flags=...) |
GIT_BRANCH_LOCAL git_branch_t |
BranchType ('Branch' would clash) |
Branches(flag=...) |
GIT_CHECKOUT_NONE git_checkout_strategy_t |
Checkout(Strategy?) | Repository.checkout(strategy=...) |
GIT_CHECKOUT_NOTIFY_NONE git_checkout_notify_t |
CheckoutNotify | CheckoutCallbacks.checkout_notify_flags() CheckoutCallbacks.checkout_notify(why=...) |
GIT_CONFIG_LEVEL_PROGRAMDATA git_config_level_t |
ConfigLevel | pygit2.settings.search_path |
GIT_CREDENTIAL_SSH_KEY git_credential_t |
CredentialType | |
GIT_DELTA_UNMODIFIED git_delta_t |
Delta (might be too broad?) |
DiffDelta.status |
GIT_DESCRIBE_DEFAULT git_describe_strategy_t |
Describe(Strategy?) | Repository.describe(describe_strategy=...) |
GIT_DIFF_FIND_BY_CONFIG git_diff_find_t |
DiffFind | Diff.find_similar(flags=...) |
GIT_DIFF_FLAG_BINARY git_diff_flag_t |
DiffFlag? | DiffDelta.flags DiffFile.flags |
GIT_DIFF_NORMAL git_diff_option_t |
DiffOption? | Blob.diff_to_buffer(flag=...) Tree.diff_to_tree(flag=...) etc. |
GIT_DIFF_STATS_NONE git_diff_stats_format_t |
DiffStatsFormat ('DiffStats' would clash) |
DiffStats.format(format=...) |
GIT_FEATURE_THREADS git_feature_t |
Feature | pygit2.features |
GIT_FETCH_PRUNE_UNSPECIFIED git_fetch_prune_t |
FetchPrune | Remote.fetch(...) |
GIT_FILEMODE_UNREADABLE git_filemode_t |
FileMode | DiffFile.mode IndexEntry.mode TreeBuilder.insert(attr=...) |
GIT_FILTER_DEFAULT git_filter_flag_t |
FilterFlag | FilterSource.flags |
GIT_FILTER_TO_WORKTREE git_filter_mode_t |
FilterMode | FilterSource.mode |
GIT_MERGE_ANALYSIS_NONE git_merge_analysis_t |
MergeAnalysis | Repository.merge_analysis() |
GIT_MERGE_PREFERENCE_NONE git_merge_preference_t |
MergePreference | Repository.merge_analysis() |
GIT_OBJECT_ANY git_object_t |
ObjectType ('Object' would clash) |
peel Object.type |
GIT_OPT_GET_MWINDOW_SIZE git_libgit2_opt_t |
Option? | _pygit2.option(option=...) |
GIT_REFERENCE_INVALID git_reference_t |
ReferenceType ('Reference' would clash) |
Reference.type |
GIT_REFERENCES_ALL (made up by pygit2) |
ReferenceFilter ('References' would clash) |
References.iterator( references_return_type=...) |
GIT_REPOSITORY_INIT_BARE git_repository_init_flag_t |
RepositoryInitFlag | pygit2.init_repository(flags=...) |
GIT_REPOSITORY_INIT_SHARED_UMASK git_repository_init_mode_t |
RepositoryInitMode ('Mode' in the chmod sense) |
pygit2.init_repository(mode=...) |
GIT_REPOSITORY_OPEN_NO_SEARCH git_repository_open_flag_t |
RepositoryOpenFlag | Repository(flags=...) |
GIT_REPOSITORY_STATE_NONE git_repository_state_t |
RepositoryState | Repository.state() |
GIT_RESET_SOFT git_reset_t |
Reset(Mode?) | Repository.reset(...) |
GIT_REVSPEC_SINGLE git_revspec_t |
RevSpecFlag ('RevSpec' would clash) |
RevSpec.flags |
GIT_SORT_NONE git_sort_t |
WalkerSort ('Sort' feels too broad to me) |
Walker.sort(...) |
GIT_STASH_APPLY_PROGRESS_NONE git_stash_apply_progress_t |
StashApplyProgress | StashApplyCallbacks.stash_apply_progress |
GIT_STATUS_CURRENT git_status_t |
FileStatus? (Would 'Status' be too broad?) |
Repository.status() |
GIT_SUBMODULE_IGNORE_UNSPECIFIED git_submodule_ignore_t |
SubmoduleIgnore | SubmoduleCollection.status(...) |
GIT_SUBMODULE_STATUS_IN_HEAD git_submodule_status_t |
SubmoduleStatus | SubmoduleCollection.status(...) |
Yet another approach might be to get rid of enums.py and move the enums to the classes they pertain to.
In this case, name clashes wouldn't matter anymore.
It might clean up the namespaces even more, but there are a couple downsides:
GIT_APPLY_LOCATION_WORKDIR --> Diff.ApplyLocation? (Repo.apply takes a Diff + an ApplyLocation)
GIT_DIFF_FIND_BY_CONFIG --> Diff.Find
GIT_DIFF_NORMAL --> Diff.Option?
GIT_DIFF_FLAG_BINARY --> DiffDelta.Flag, DiffFile.Flag? (used by both)
GIT_DELTA_UNMODIFIED --> DiffDelta.Status
GIT_FILTER_DEFAULT --> FilterSource.Flag (I couldn't find unit tests for those)
GIT_FILTER_TO_WORKTREE --> FilterSource.Mode
GIT_SUBMODULE_IGNORE_UNSPECIFIED--> SubmoduleCollection.Ignore
GIT_SUBMODULE_STATUS_IN_HEAD --> SubmoduleCollection.Status
GIT_ATTR_CHECK_FILE_THEN_INDEX --> Repository.AttrCheck
GIT_CHECKOUT_NONE --> Repository.Checkout(Strategy?)
GIT_DESCRIBE_DEFAULT --> Repository.Describe(Strategy?)
GIT_MERGE_ANALYSIS_NONE --> Repository.MergeAnalysis
GIT_MERGE_PREFERENCE_NONE --> Repository.MergePreference
GIT_REPOSITORY_INIT_BARE --> Repository.InitFlag
GIT_REPOSITORY_INIT_SHARED_UMASK--> Repository.InitMode
GIT_REPOSITORY_OPEN_NO_SEARCH --> Repository.OpenFlag
GIT_REPOSITORY_STATE_NONE --> Repository.State
GIT_RESET_SOFT --> Repository.Reset
GIT_BLAME_NORMAL --> Blame.Flag
GIT_BLOB_FILTER_CHECK_FOR_BINARY--> BlobIO.Filter
GIT_BRANCH_LOCAL --> Branches.Filter
GIT_CHECKOUT_NOTIFY_NONE --> CheckoutCallbacks.Notify
GIT_DIFF_STATS_NONE --> DiffStats.Format
GIT_FETCH_PRUNE_UNSPECIFIED --> Remote.FetchPrune
GIT_REFERENCES_ALL --> References.Filter
GIT_REFERENCE_INVALID --> Reference.Type
GIT_REVSPEC_SINGLE --> RevSpec.Flag
GIT_STASH_APPLY_PROGRESS_NONE --> StashApplyCallbacks.Progress
GIT_SORT_NONE --> Walker.Sort
GIT_CONFIG_LEVEL_PROGRAMDATA --> (root).ConfigLevel?
GIT_FEATURE_THREADS --> (root).Feature
GIT_FILEMODE_UNREADABLE --> (root).FileMode (used by multiple classes)
GIT_STATUS_CURRENT --> (root).FileStatus?
GIT_OBJECT_ANY --> (root).GitObjectType?
GIT_OPT_GET_MWINDOW_SIZE --> (root).Option?
Hi @jorio
I agree with your points (all in pygit2.enums
and the name must be unique).
Here a proposal, it's a synthesis of your two proposals:
ApplyLocation
AttrCheck
BlameFlag
BlobFilter
BranchType
CheckoutStrategy
CheckoutNotify
ConfigLevel
CredentialType
DeltaStatus
DescribeStrategy
DiffFind
DiffFlag
DiffOption
DiffStatsFormat
Feature
FetchPrune
FileMode
FilterFlag
FilterMode
MergeAnalysis
MergePreference
ObjectType
Option
ReferenceType
ReferenceFilter
RepositoryInitFlag
RepositoryInitMode
RepositoryOpenFlag
RepositoryState
ResetMode
RevSpecFlag
SortMode *
StashApplyProgress
FileStatus
SubmoduleIgnore
SubmoduleStatus
Only change of mine is that I've changed WalkerSort
to SortMode
.
So the general rule would be:
Flag
, Type
or Mode
With few exceptions: FileStatus
because it makes sense, Option
instead of Opt
.
If the list of names above looks good, then you can go ahead with the PR. Or tell me if you still want to make changes. Thanks
Hi @jdavid, this looks good to me! I'll amend the PR with the new names soon-ish. Thanks again.
Hi @jdavid, I rebased the PR on master with the name changes that you suggested.
Additional changes since last time:
Let me know if there's anything that still needs tweaking. Thank you!
Thanks @jorio !
This PR introduces "pythonic" IntEnum and IntFlag classes that can supersede
GIT_...
integers in user code.The main benefit of these enum classes is that they naturally guide the user toward the set of flags you can pass to a function, instead of having the user trawl through a list of GIT_ constants. Some examples:
Another benefit is that the new enums are augmented with docstrings. I'm a heavy user of pygit2 and I tend to frequently have to dive into libgit2's source code to recall what some flags do – the new enums reduce this friction:
Notes:
StatusFlags(1<<31)
will still work and it'll convert to a valid int.Repository.state()
(where I just return a raw int if it can't be converted to a RepositoryState).If you think this PR worth merging into pygit2, I'm happy to discuss naming choices, etc.
Next steps: If this PR is accepted, I will submit another one later that makes some C functions return actual Python enums. This way, functions such as
repo.status_file(...)
will return a real StatusFlags instead of an int, which would make it easier to toy with pygit2 in an REPL.