-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
feat: started working on auto-commit #441
base: master
Are you sure you want to change the base?
Conversation
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.
👍 Looks good to me! Reviewed everything up to 02a7ebf in 1 minute and 2 seconds
More details
- Looked at
59
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. gptme/chat.py:250
- Draft comment:
Consider logging when auto-commit is invoked and ensuring the env config check is case-insensitive. This will help with debugging and avoid subtle config issues. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. gptme/util/context.py:338
- Draft comment:
The autocommit() stub currently returns a placeholder message. Consider raising a NotImplementedError or adding more concrete implementation safeguards. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. gptme/chat.py:244
- Draft comment:
The nested if structure for modification checks is clearer. Consider normalizing the GPTME_AUTOCOMMIT env var (e.g. using .lower()) for case-insensitive comparison, and add logging when auto-commit is triggered. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. gptme/util/context.py:335
- Draft comment:
The new autocommit() function is a placeholder. Consider adding more explicit logging or notes until the full auto-commit implementation is provided. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_EkFdv19PamIpwXL2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
02a7ebf
to
93608dc
Compare
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.
❌ Changes requested. Incremental review on 93608dc in 1 minute and 15 seconds
More details
- Looked at
60
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. gptme/util/context.py:338
- Draft comment:
The auto-commit feature is a placeholder. Consider raising a NotImplementedError or adding more contextual logging to guide future development. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The function is clearly a placeholder with TODOs, but it does communicate its unimplemented state through the return message. While raising NotImplementedError would be more "proper", the current implementation actually provides more user-friendly feedback. The suggestion is a minor implementation detail that doesn't materially improve the code.
The comment does point out a valid software engineering best practice. NotImplementedError is the standard Python way to handle unimplemented features.
However, the current implementation with a user-friendly message is arguably better for this use case than raising an exception. The TODOs already provide good context for future development.
The comment should be deleted as it suggests changing something that is already handled appropriately for this specific use case.
2. gptme/chat.py:243
- Draft comment:
When checking the GPTME_AUTOCOMMIT env variable, consider normalizing it (e.g. using .lower()) to handle different capitalizations. Also, ensure the ordering between checking for pre-commit failures and auto-commit is as intended. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. gptme/util/context.py:338
- Draft comment:
The autocommit() function is a clear stub with TODOs. Ensure that before enabling this feature, proper staging, commit message generation, and a review step are implemented to avoid unintended commits (especially in production). - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_nK7i94grjaGhAo7F
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
if failed_check_message := check_changes(): | ||
yield Message("system", failed_check_message, quiet=False) | ||
return | ||
if get_config().get_env("GPTME_AUTOCOMMIT") in ["true", "1"]: |
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.
The GPTME_AUTOCOMMIT check is case-sensitive. Consider normalizing (e.g., lowercasing) the env value to avoid misconfiguration.
if get_config().get_env("GPTME_AUTOCOMMIT") in ["true", "1"]: | |
if get_config().get_env("GPTME_AUTOCOMMIT").lower() in ["true", "1"]: |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #441 +/- ##
==========================================
- Coverage 67.16% 67.11% -0.05%
==========================================
Files 71 71
Lines 6304 6310 +6
==========================================
+ Hits 4234 4235 +1
- Misses 2070 2075 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Since the experiment with integrating pre-commit checks wen't well, I figured I should attempt to add an autocommit feature as well. Would speed up many of my workflows, especially with @TimeToBuildBob.
Important
Introduces a placeholder
autocommit
feature incontext.py
and integrates it intochat.py
, controlled byGPTME_AUTOCOMMIT
environment variable.autocommit()
function incontext.py
as a placeholder for future auto-commit functionality.autocommit()
intostep()
inchat.py
, triggered ifGPTME_AUTOCOMMIT
is "true" or "1".GPTME_AUTOCOMMIT
environment variable to control auto-commit feature.This description was created by
for 93608dc. It will automatically update as commits are pushed.