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.

Sign up for GitHub

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

Jump to bottom

refactoring all json utilities #2032

Merged
merged 7 commits into from
Apr 19, 2023
Merged

refactoring all json utilities #2032

merged 7 commits into from
Apr 19, 2023

Conversation

bingoko
Copy link
Contributor

@bingoko bingoko commented Apr 16, 2023

Background

This pull request addresses the need to refactor and consolidate all JSON-related functions into a few utility files under the JSON Utils module. Previously, these functions were scattered across various JSON modules and functions, making it difficult to maintain and understand the codebase. This change is in line with the project's overall goal of improving maintainability, readability, and structure.

Changes

  • Created a new json_utils module to house all JSON-related utility functions.
  • Moved existing JSON functions from their scattered locations into the json_utils module.
  • Refactored and optimized the JSON functions for better readability and maintainability.

Documentation

  • Added in-code comments to explain the purpose and functionality of each JSON utility function in the json_utils module.

Test Plan

  • Manually tested each JSON utility function with a variety of JSON inputs to ensure they function as expected.
  • Updated existing unit tests to reference the new module and function locations.
  • No new tests were added since the changes only involved refactoring existing functions and no new functionality was introduced.

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

@nponeccop nponeccop added the invalid_json Groups issues and PRs related to invalid json error or similar label Apr 17, 2023
@nponeccop
Copy link
Contributor

@merwanehamadi

@waynehamadi
Copy link
Contributor

waynehamadi commented Apr 17, 2023 •
edited
Loading

@bingoko Great thanks a lot. I tested it and also the unit tests keep passing.
There is just one issue I don't know if it also bugs you:

  • our unit tests are not testing the right method that wraps everything
  • our unit tests call the ai magic function which means it's going to cost tokens or we're going to need to mock that. It would be so nice to have the a separate ai function from the non ai functions, so we can load as many unit tests as we need on the non ai function.

But this approved, if you could help us with what's above after this is merged, that would be awesome

PS: linter issue

waynehamadi
waynehamadi previously approved these changes Apr 17, 2023
View reviewed changes
@bingoko
Copy link
Contributor Author

bingoko commented Apr 17, 2023

You're welcome! I've taken your feedback into account and have now split the functions into AI and non-AI functions. This should make it easier to test the non-AI functions without incurring token costs or the need to mock the AI magic function.

Cheers!

@bingoko bingoko requested a review from waynehamadi April 17, 2023 11:18
@nponeccop nponeccop added the B8 label Apr 17, 2023
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 17, 2023
@github-actions
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@bingoko
Merge remote-tracking branch 'upstream/master'
31900f6 
# Conflicts:
#	autogpt/app.py
#	autogpt/json_fixes/auto_fix.py
#	autogpt/json_fixes/bracket_termination.py
#	autogpt/json_fixes/master_json_fix_method.py
#	autogpt/json_utils/json_fix_llm.py
#	autogpt/json_utils/utilities.py
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Apr 17, 2023
@github-actions
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@waynehamadi
Copy link
Contributor

@bingoko I love you

@waynehamadi
Copy link
Contributor

@bingoko I have an issue running the agent
Screenshot 2023-04-17 at 4 09 08 PM

@bingoko
Copy link
Contributor Author

bingoko commented Apr 17, 2023

@bingoko I have an issue running the agent Screenshot 2023-04-17 at 4 09 08 PM

done

waynehamadi
waynehamadi previously approved these changes Apr 17, 2023
View reviewed changes
@BillSchumacher BillSchumacher merged commit ecf2ba1 into Significant-Gravitas:master Apr 19, 2023
@jihaui jihaui mentioned this pull request Nov 24, 2023
Open
@hxs9712 hxs9712 mentioned this pull request Dec 1, 2023
Open
@jimjonesbabyfreshout jimjonesbabyfreshout mentioned this pull request Dec 5, 2023
Open
@MyDevops101 MyDevops101 mentioned this pull request Feb 8, 2024
Open
@dewankpant dewankpant mentioned this pull request Feb 10, 2024
Open
@matr1xp matr1xp mentioned this pull request Apr 29, 2024
Open
sindlinger pushed a commit to Orgsindlinger/Auto-GPT-WebUI that referenced this pull request Sep 25, 2024
@BillSchumacher
Merge pull request Significant-Gravitas#2032 from bingoko/master
c8820a0 
refactoring all json utilities
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillSchumacher BillSchumacher BillSchumacher approved these changes

@waynehamadi waynehamadi waynehamadi left review comments

Assignees

@BillSchumacher BillSchumacher

Labels
invalid_json Groups issues and PRs related to invalid json error or similar
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bingoko @nponeccop @waynehamadi @BillSchumacher