- Notifications
You must be signed in to change notification settings - Fork11
More:git init#487
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
More:git init#487
Conversation
sourcery-aibot commentedFeb 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Reviewer's Guide by SourceryThis PR enhances the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess yourdashboard to:
Getting Help
|
codecovbot commentedFeb 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## master #487 +/- ##==========================================+ Coverage 54.05% 56.30% +2.24%========================================== Files 40 40 Lines 3637 3799 +162 Branches 794 808 +14 ==========================================+ Hits 1966 2139 +173+ Misses 1319 1306 -13- Partials 352 354 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey@tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a helper function to reduce the duplication in the validation logic for the
sharedparameter. - The
initial_branchparameter is an alias forbranch, but it's not clear from the code that they are interchangeable - consider explicitly settingbranch = initial_branch.
Here's what I looked at during the review
- 🟡General issues: 2 issues found
- 🟢Security: all looks good
- 🟡Testing: 2 issues found
- 🟡Complexity: 1 issue found
- 🟢Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ifsharedisnotNone: | ||
| valid_shared_values= { | ||
| "false", | ||
| "true", | ||
| "umask", | ||
| "group", | ||
| "all", | ||
| "world", | ||
| "everybody", | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
issue (bug_risk): Boolean shared value check appends flag even for false.
The updated code checks if shared is an instance of bool and then unconditionally appends "--shared". This means that if shared is False (explicitly turning off sharing), the flag will still be added. To maintain behavior consistent with earlier logic—where the flag was only added for True—it would be better to explicitly check that shared is True (e.g. usingif shared is True instead).
| "or a valid octal number between 0000 and 0777" | ||
| ) | ||
| raiseValueError(msg) | ||
| local_flags.append(f"--shared={shared}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
suggestion (bug_risk): Consistency in handling non-boolean shared values.
After converting the non-boolean shared value to lower case (stored in shared_str) for validation, the flag is appended using the original shared variable rather than the normalized one. Consider using the lowercased value in constructing the flag to ensure consistent representation of the flag value.
| local_flags.append(f"--shared={shared}") | |
| local_flags.append(f"--shared={shared_str}") |
| result=repo.init(bare=True) | ||
| assert"Initialized empty Git repository"inresult | ||
| # Bare repos have files directly in the directory | ||
| assert (tmp_path/"HEAD").exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
suggestion (testing): Missing assertion for the--bare flag's effect on config.
While the existence ofHEAD is a good indicator, it's not definitive proof of a bare repository. A stronger assertion would be to check thecore.bare config setting within the.git directory. This ensures the repository is truly configured as bare.
| assert (tmp_path/"HEAD").exists() | |
| assert (tmp_path/"HEAD").exists() | |
| config_path=tmp_path/"config" | |
| assertconfig_path.exists(),"Config file does not exist in bare repository" | |
| config_text=config_path.read_text() | |
| assert"bare = true"inconfig_text,"Repository core.bare flag not set to true" |
| # Note: sha256 test is commented out as it might not be supported in all | ||
| # git versions | ||
| # repo_dir = tmp_path / "sha256" | ||
| # repo_dir.mkdir() | ||
| # repo = git.Git(path=repo_dir) | ||
| # result = repo.init(object_format="sha256") | ||
| # assert "Initialized empty Git repository" in result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
suggestion (testing): Handle potential unsupportedsha256.
Instead of commenting out thesha256 test, usepytest.mark.skipif to conditionally skip the test if the current Git version doesn't supportsha256. This provides a cleaner way to handle unsupported features and keeps the test suite more comprehensive.
Suggested implementation:
importpathlibimportgitimportpytestimportsubprocessdefsupports_sha256()->bool:try:output=subprocess.check_output(["git","--version"],universal_newlines=True)version_str=output.strip().split()[2]major,minor,*_=map(int,version_str.split('.'))# SHA256 support is assumed for Git versions 2.29 and above.return (major,minor)>= (2,29)exceptException:returnFalse
deftest_git_init_object_format(tmp_path:pathlib.Path)->None:"""Test git init with different object formats."""repo=git.Git(path=tmp_path)# Test with sha1 (default)result=repo.init(object_format="sha1")assert"Initialized empty Git repository"inresultassertrepo.path==tmp_path
@pytest.mark.skipif(notsupports_sha256(),reason="sha256 not supported by current git version")deftest_git_init_object_format_sha256(tmp_path:pathlib.Path)->None:"""Test git init with sha256 object format."""repo_dir=tmp_path/"sha256"repo_dir.mkdir()repo=git.Git(path=repo_dir)result=repo.init(object_format="sha256")assert"Initialized empty Git repository"inresult
Make sure that the new helper function supports_sha256() and the import for pytest and subprocess do not duplicate existing imports in your project. Adjust the version threshold if your requirements are different.
| ) | ||
| definit( | ||
| self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
issue (complexity): Consider extracting parameter validation logic into dedicated helper functions to reduce nesting and improve readability.
Consider extracting parameter validation logic into dedicated helper functions to simplify the nesting. For example, you can extract the template and shared validations. This will make theinit method shorter and easier to read without changing its behavior.
Example for extracting template validation:
def_validate_template(template:str|pathlib.Path)->str:ifnotisinstance(template, (str,pathlib.Path)):raiseTypeError("template must be a string or Path")template_path=pathlib.Path(template)ifnottemplate_path.is_dir():raiseValueError(f"template directory does not exist:{template}")returnf"--template={template}"
Example for extracting shared validation:
def_validate_shared(shared:bool|t.Literal["false","true","umask","group","all","world","everybody"]|str|None)->str|None:ifsharedisNone:returnNonevalid_shared_values= {"false","true","umask","group","all","world","everybody"}ifisinstance(shared,bool):return"--shared"ifsharedelseNoneshared_str=str(shared).lower()ifnot (shared_strinvalid_shared_valuesor (shared_str.isdigit()andlen(shared_str)<=4andall(cinstring.octdigitsforcinshared_str)andint(shared_str,8)<=0o777 ) ):raiseValueError(f"Invalid shared value. Must be one of{valid_shared_values} or a valid octal number between 0000 and 0777" )returnf"--shared={shared}"
Then, in theinit method, you can simplify the code:
iftemplateisnotNone:local_flags.append(_validate_template(template))# ... other flags ...ifsharedisnotNone:shared_flag=_validate_shared(shared)ifshared_flag:local_flags.append(shared_flag)
This refactoring reduces nesting and centralizes validation logic, making it easier to maintain and test.
| valid_shared_values= { | ||
| "false", | ||
| "true", | ||
| "umask", | ||
| "group", | ||
| "all", | ||
| "world", | ||
| "everybody", | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
suggestion (code-quality): We've found these issues:
- Move assignments closer to their usage (
move-assign) - Low code quality found in Git.init - 11% (
low-code-quality)
| valid_shared_values= { | |
| "false", | |
| "true", | |
| "umask", | |
| "group", | |
| "all", | |
| "world", | |
| "everybody", | |
| } | |
| valid_shared_values= { | |
| "false", | |
| "true", | |
| "umask", | |
| "group", | |
| "all", | |
| "world", | |
| "everybody", | |
| } |
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
- Add support for all git-init options (template, separate_git_dir, object_format, etc.)- Add comprehensive tests for each option- Fix path handling for separate_git_dir- Fix string formatting for bytes paths- Update docstrings with examples for all options
- Enhance Git.init docstrings with detailed parameter descriptions- Add comprehensive examples including SHA-256 object format- Add return value and exception documentation- Improve type hints for shared parameter with Literal types- Add extensive validation tests for all parameters
- Add validation for template parameter type and existence- Add validation for object_format parameter values- Improve type formatting for shared parameter- Complete docstring example output
- Add ref-format parameter support for git init- Add make_parents parameter to control directory creation- Improve type hints and validation for template and shared parameters- Add comprehensive tests for all shared values and octal permissions- Add validation for octal number range in shared parameter
Uh oh!
There was an error while loading.Please reload this page.
Changes
Cmd:
Git.initimprovementsThis PR enhances the
Git.init()method with improved parameter validation, comprehensive documentation, and extensive test coverage. Key improvements include:Enhanced Parameter Support:
ref_formatparameter (files/reftable)defaultparameter for default permissionsmake_parentsparameter to control directory creationsharedparameter to support all git-supported valuesImproved Type Handling:
templateparameter to accept bothstrandpathlib.PathDocumentation Improvements:
Validation & Error Handling:
Examples
Testing
The PR includes comprehensive test coverage. To run the tests:
Validation Tests
The PR includes tests for various validation scenarios:
Breaking Changes
None. All changes are backward compatible while adding new functionality.
Dependencies
ref_format="reftable") require git >= 2.37.0Summary by Sourcery
Enhances the
Git.init()method with improved parameter validation, comprehensive documentation, and extensive test coverage. It introduces new features such as support forref_format,default, andmake_parentsparameters, and enhances existing parameters liketemplateandsharedwith improved type handling and validation.New Features:
ref_formatparameter to specify the reference storage format.defaultparameter to use default permissions for directories and files.make_parentsparameter to create parent directories if they don't exist.Enhancements:
templateparameter to accept bothstrandpathlib.Pathtypes.sharedparameter to support all git-supported values, including boolean, string literals, and octal number strings.template,object_format,branch, andsharedparameters.Tests:
git initcommand, including tests for various parameters and validation scenarios.