[ACIX-1060] Add classes for handling git objects: Commit, ChangeSet...
#184
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ishirui
force-pushed
the
pierrelouis.veyrenc/ACIX-1035-git-related-commands-classes
branch
from
fa217f9 to
b6ff2ce
Compare
Ishirui
force-pushed
the
pierrelouis.veyrenc/ACIX-1035-git-related-commands-classes
branch
2 times, most recently
from
6456bbc to
5d62a2e
Compare
Base automatically changed from
pierrelouis.veyrenc/ACIX-1035-git-tool-and-config
to
main
September 11, 2025 16:20
Ishirui
force-pushed
the
pierrelouis.veyrenc/ACIX-1035-git-related-commands-classes
branch
from
5d62a2e to
73a49be
Compare
Ishirui
changed the title
Commit, ChangeSet...Commit, ChangeSet...
ofek
requested changes
Sep 18, 2025
Ishirui
force-pushed
the
pierrelouis.veyrenc/ACIX-1035-git-related-commands-classes
branch
from
d481e6b to
f0256f5
Compare
ofek
requested changes
Sep 19, 2025
refactor(tests): Move fixtures to `git` folder feat(git): Add `commit_files` method replacing `tests.helpers.commit_file` + test changes fix(tests): Use `default_git_author` fixture in tests
We can actually do this quite easily by avoiding the use of `_details` and `_changes` fields, and instead using `__dict__` to store the details and changes. This is the same technique that `cached_property` uses internally, so trying to access the cached property works as expected if the details or changes have been set by one of the `get_details_from_*` or `get_changes_from_*` methods.
Spurred on by [this review comment](#184 (comment)). Not having access to `Application` here means the `get_changes_from_git` and `get_details_from_git` are not possible to implement. This then causes a cascade of things: - Only having one source for `details` and `changes` means we can just use standard `@cached_properties` - However, we don't want to favorize people using the Github API instead of calls to git as it is less reliable. - To prevent this, the easiest is just to remove the `details` and `changes` properties altogether. Callers can still access these by using the forward-relationship methods `Git.get_commit_details` or `Remote.get_commit_details_and_changes` - To keep a consistent style and avoid confusing people, we remove the other reverse-relationship methods (`Commit.head` - `Git.get_head_commit` can be used instead. Same for `compare_to`) - We can then remove all the proxying methods. - Since we don't need to encode `ChangeSet`s, the `enc_hooks` are not needed anymore, `Commit`s are natively encodable + Remove extraneous tests and fix remaining ones
Ishirui
force-pushed
the
pierrelouis.veyrenc/ACIX-1035-git-related-commands-classes
branch
from
528e48d to
2100703
Compare
Unfortunately `msgspec` does not natively support encoding/decoding `MappingProxyType` objects, so we need to implement custom logic for that: jcrist/msgspec#632
ofek
requested changes
Sep 25, 2025
tests/tools/git/fixtures/repo_states/complex_changes/expected_changeset.json
Outdated
Show resolved
Hide resolved
- Store both author and commiter details - Store these as `tuple`s - Store the commit date as a UNIX timestamp and only convert to `datetime` when needed - Tweak call to `git show` format + use `NUL` characters for splitting - Remove `parent_shas` list
This makes instantiation a bit clunkier (as can be seen in tests), but directly instantiating a `Commit` will be rare, more often getting it from a `git` command or GitHub.
- Remove "file structure" comments - Lazily import a couple of things
We also now encode Path objects as simple `str`s instead of encoding them as their `repr` - this requires a change to test data.
This is a lot cleaner and easier to use. Thanks @ofek for the suggestion !
Ishirui
deleted the
pierrelouis.veyrenc/ACIX-1035-git-related-commands-classes
branch
github-actions bot
pushed a commit
that referenced
this pull request
Sep 26, 2025
…t`... (#184) * Port src/ changes from #181 * Port `tests/` changes from #181 * fix(git): Fix tests - move fixtures and use proper author details refactor(tests): Move fixtures to `git` folder feat(git): Add `commit_files` method replacing `tests.helpers.commit_file` + test changes fix(tests): Use `default_git_author` fixture in tests * feat(git): Add `Commit.enc_hook` and `Commit.dec_hook` * refactor(git): Make `Commit` objects frozen We can actually do this quite easily by avoiding the use of `_details` and `_changes` fields, and instead using `__dict__` to store the details and changes. This is the same technique that `cached_property` uses internally, so trying to access the cached property works as expected if the details or changes have been set by one of the `get_details_from_*` or `get_changes_from_*` methods. * refactor(git): Remove `commit_files` method * refactor(git): Minor fixes * refactor(git): Rename `FileChanges` to `ChangedFile` * refactor(git): Move everything related to GitHub to own file * refactor(git): Replace `__new__` override with explicit constructor * fix(git): Check for failures when calling `git diff` * refactor(git): Extract `add` and `commit` into individual methods * feat(git): Add `binary` field to `FileChanges` objects * refactor(git): Use regex-based parsing method for `generate_from_diff_output` * fix(git): Only validate length in `Commit` class * refactor(git): Simplify `Commit` class Spurred on by [this review comment](#184 (comment)). Not having access to `Application` here means the `get_changes_from_git` and `get_details_from_git` are not possible to implement. This then causes a cascade of things: - Only having one source for `details` and `changes` means we can just use standard `@cached_properties` - However, we don't want to favorize people using the Github API instead of calls to git as it is less reliable. - To prevent this, the easiest is just to remove the `details` and `changes` properties altogether. Callers can still access these by using the forward-relationship methods `Git.get_commit_details` or `Remote.get_commit_details_and_changes` - To keep a consistent style and avoid confusing people, we remove the other reverse-relationship methods (`Commit.head` - `Git.get_head_commit` can be used instead. Same for `compare_to`) - We can then remove all the proxying methods. - Since we don't need to encode `ChangeSet`s, the `enc_hooks` are not needed anymore, `Commit`s are natively encodable + Remove extraneous tests and fix remaining ones * refactor(git): Make `ChangeSet.changes` public and immutable Unfortunately `msgspec` does not natively support encoding/decoding `MappingProxyType` objects, so we need to implement custom logic for that: jcrist/msgspec#632 * feat(git): Use SHA256 for `ChangeSet.digest` * refactor(git): Rename `Git.get_remote_details` to `Git.get_remote` * refactor(git): Tweak implementation of `CommitDetails` - Store both author and commiter details - Store these as `tuple`s - Store the commit date as a UNIX timestamp and only convert to `datetime` when needed - Tweak call to `git show` format + use `NUL` characters for splitting - Remove `parent_shas` list * refactor(git): Use `capture` instead of `run` * refactor(git): Merge the `CommitDetails` class into Commit This makes instantiation a bit clunkier (as can be seen in tests), but directly instantiating a `Commit` will be rare, more often getting it from a `git` command or GitHub. * refactor(git): Pack author and committer details into a `GitPersonDetails` struct * fix(git): Fix recurring typo `commiter` -> `committer` * refactor(git): Minor tweaks - Remove "file structure" comments - Lazily import a couple of things * feat(git): Use a single set of public methods for encoding and decoding We also now encode Path objects as simple `str`s instead of encoding them as their `repr` - this requires a change to test data. * refactor(git): Rename `ChangedFile.file` -> `ChangedFile.path` * refactor(git): Make `ChangeSet` a standard Python class rather than `Struct` * refactor(git): Move `generate_from_diff_output` logic to `ChangeSet` * refactor(git): Move and rename `Remote.get_commit_and_changes_from_remote` * feat(git): Expand support to all types of git remotes * feat(git): Replace diff-related methods with single `Git.get_changes` This is a lot cleaner and easier to use. Thanks @ofek for the suggestion ! b78a5e1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.