Skip to content
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

Fix clone_repository to conform to URL validation #3150

Merged
merged 2 commits into from
Apr 29, 2023

Conversation

SBNovaScript
Copy link
Contributor

Background

In the most recent change, git clone no longer adheres to the URL validation schema, causing positional argument errors.

Changes

Changed repository_url to url to adhere to the URL validation schema.

Documentation

Updated the function documentation to say url instead.

Test Plan

Same tests as before.

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: -8.53 ⚠️

Comparison is base (29284a5) 49.52% compared to head (6019a55) 41.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3150      +/-   ##
==========================================
- Coverage   49.52%   41.00%   -8.53%     
==========================================
  Files          64       64              
  Lines        2946     2946              
  Branches      497      497              
==========================================
- Hits         1459     1208     -251     
- Misses       1365     1679     +314     
+ Partials      122       59      -63     
Impacted Files Coverage Δ
autogpt/commands/git_operations.py 0.00% <0.00%> (ø)

... and 15 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

edcohen08
edcohen08 previously approved these changes Apr 24, 2023
Copy link
Contributor

@edcohen08 edcohen08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! out of curiosity did you actually get an error? i thought that even though it's called url in the wrapper function because that's a positional arg and not a kwarg that it doesn't actually matter what the url argument is called, just that it comes first

@SBNovaScript
Copy link
Contributor Author

lgtm! out of curiosity did you actually get an error? i thought that even though it's called url in the wrapper function because that's a positional arg and not a kwarg that it doesn't actually matter what the url argument is called, just that it comes first

Yep, I did! I received an error stating that the positional argument url was missing.

talkahe
talkahe previously approved these changes Apr 24, 2023
@edcohen08
Copy link
Contributor

edcohen08 commented Apr 24, 2023 via email

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (b8478a9) 57.19% compared to head (2d99230) 57.19%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3150   +/-   ##
=======================================
  Coverage   57.19%   57.19%           
=======================================
  Files          68       68           
  Lines        3079     3079           
  Branches      516      516           
=======================================
  Hits         1761     1761           
  Misses       1180     1180           
  Partials      138      138           
Impacted Files Coverage Δ
autogpt/commands/git_operations.py 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@SBNovaScript SBNovaScript force-pushed the fix-git-clone branch 2 times, most recently from ed39335 to 2540c9f Compare April 25, 2023 09:37
@talkahe talkahe mentioned this pull request Apr 25, 2023
1 task
@SBNovaScript SBNovaScript force-pushed the fix-git-clone branch 3 times, most recently from 8f21e0d to 67abf1b Compare April 26, 2023 18:01
@vercel
Copy link

vercel bot commented Apr 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Apr 29, 2023 0:42am

Copy link

@talkahe talkahe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested and git clonecommand is working again. LGTM!
@Pwuts @BillSchumacher

Copy link
Contributor

@edcohen08 edcohen08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@k-boikov k-boikov added bug Something isn't working high priority labels Apr 28, 2023
@Pwuts
Copy link
Member

Pwuts commented Apr 29, 2023

If one of you has the time, please also submit a PR adding a test for clone_repository so this doesn't happen again :)

@Pwuts Pwuts changed the title Fix git clone to conform to validation. Fix clone_repository to conform to URL validation Apr 29, 2023
@Pwuts Pwuts merged commit 9c6494a into Significant-Gravitas:master Apr 29, 2023
dschonholtz pushed a commit to dschonholtz/Auto-GPT that referenced this pull request Apr 29, 2023
…itas#3150)

Co-authored-by: Reinier van der Leer <github@pwuts.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority size/m
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants