-
Notifications
You must be signed in to change notification settings - Fork 536
Document and Announce the git gem's architectural redesign #834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
Update the CI builds to build with MRI Ruby 3.2, 3.3, and 3.4; TruffleRuby 24.2.1; and JRuby 10.0.0.1. BREAKING CHANGE: Users will need to be on Ruby 3.2 or greater
This partially implements #813 Log data access methods directly on the Log class will return a deprecation warning since they will be removed in the future.
This pull request refactors the Git::Diff decomposing it into new, more focused classes, while backward compatibility is maintained via a deprecated facade.
This will match all my other projects to aid copy and pasting between projects.
The Branch, Remote, and Worktree classes all inherit from Path but do not initialize it. Rather than call super, I am removing the inheritance. While this could be considered a breaking change, since the base class was never initialized, objects of these classes can not make use of the Path methods.
Ignore this offense for the gemspec and for tests since the DSL for both make it hard to limit the block length.
``` ssh-keygen -t dsa unknown key type dsa ``` with OpenSSH_9.9p1
Most of this was fixed by excluding tests from this metric. This is fine since I plan on moving tests to RSpec in the near future. The other few instances were fixed by refactoring the methods.
Exclude tests from the Metrics/ClassLength Rubocop offfense since the plan is to rewrite the test suite in RSpec. At that time many of the Rubocop exclusions will be removed.
This refactoring streamlines the Git::Status class by decomposing its responsibilities, resulting in cleaner, more focused components: * `StatusFile`: The inner data responsibilities was moved to its own class. * `StatusFileFactory`: A new private factory was created to encapsulate all the logic for executing git commands and parsing their output into StatusFile objects. I think the result is more readable and maintainable code.
The Git::Log class had grown to over 150 lines, making it difficult to read and maintain. This refactoring simplifies the internal implementation to improve its structure and reduce its size to under 100 lines, without altering the public API. The primary motivations were to enhance readability and make the class easier to extend in the future. The key changes include: - Consolidated all query parameters from individual instance variables into a single `@options` hash to simplify state management. - Replaced repetitive builder methods with a concise `set_option` private helper, reducing code duplication. - Centralized the command execution logic into a single `run_log_if_dirty` method to ensure consistent behavior. - Simplified the deprecated Enumerable methods by using a helper for warnings and safe navigation (`&.`) for cleaner error handling. - Modernized the nested `Git::Log::Result` class to use `Data.define`, leveraging Ruby 3.2+ features to create a more concise and immutable result object.
(bin/test prepends the unit_test directory; the quoted calls previously would fail because it ended up trying to run `tests/units/test/units/test_archive.rb`)
Use the keyword parameter, not the positional one.
These methods were deprecated with the same thinking that the Git::Log methods were deprecated. However, where Git::Log is a query builder, Git::Diff is (mostly) not... it is a facade over diff, diff_stats, and diff_path_stats. A problem remains with the Git::Diff class in that if you call the #path method after retrieving results, the results are not updated which may cause unexpected results. I'll consider what changes should be made to the Git::Diff class at a later date.
ActiveSupport::Deprecation is designed to report the line where the deprecated method is called (one level up the call stack), not where the warning itself is defined. The previous implementation triggered warnings from an internal helper method, causing them to report the location with the Git gem. This commit moves the `warn` call into the public-facing deprecated method, ensuring the warning correctly points to the user's code that should be changed.
Correctly report that the current deprecations will be removed by version 5.0.0 of the git gem.
this is so that non-deprecated use of this gem will not produce deprecation warnings that the user can't fix.
Several methods, arguments, and behaviors were deprecated but the tests were not updated to use alternates. This caused a lot of deprecation warnings when running the tests.
Deprecation warnings should not be ignored. This is important so that: * when a user sees a deprecation warning, they can be confident it is coming from their code and not this gem * test output is clean and does not contain noisey deprecation warnings Tests whose purpose is to test that a deprecation warning is issued in the right circumstance should mock Git::Deprecation#warn to avoid raising an error.
This class is internal only as well as the classes that inherit from it (Git::Index and Git::WorkingTree). Remove the deprecation warning and just remove the deprecated code.
Since Git::Stash#initialize is an internal only method, remove the deprecation warning and removed the deprecated code.
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.
The git gem is undergoing a significant architectural redesign for the upcoming
v5.0.0 release. The current architecture has several design challenges that make it
difficult to maintain and evolve. This redesign aims to address these issues by
introducing a clearer, more robust, and more testable structure.
We have prepared detailed documents outlining the analysis of the current
architecture and the proposed changes. We encourage our community and contributors to
review them:
breakdown of the existing design and its challenges.
new three-layered architecture.
plan for implementing the redesign.
Your feedback is welcome! Please feel free to open an issue to discuss the proposed
changes.