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

move remove_color_codes to utils and add tests #3260

Merged
merged 6 commits into from
Apr 28, 2023
Merged

move remove_color_codes to utils and add tests #3260

merged 6 commits into from
Apr 28, 2023

Conversation

k-boikov
Copy link
Contributor

Background

Received a complain from team member for potential fails in remove_color_codes, so tests were created.

Changes

Moved remove_color_codes to utils and added tests.

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

@k-boikov k-boikov force-pushed the improvement/remove-colors-refactor-plus-tests branch from c5ef99d to d6ba1c2 Compare April 25, 2023 22:18
@codecov
Copy link

codecov bot commented Apr 25, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (aebe891) 56.46% compared to head (0a4754a) 56.46%.

❗ Current head 0a4754a differs from pull request most recent head bc6bc07. Consider uploading reports for the commit bc6bc07 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3260   +/-   ##
=======================================
  Coverage   56.46%   56.46%           
=======================================
  Files          65       65           
  Lines        3018     3018           
  Branches      507      507           
=======================================
  Hits         1704     1704           
  Misses       1175     1175           
  Partials      139      139

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

@k-boikov k-boikov force-pushed the improvement/remove-colors-refactor-plus-tests branch 2 times, most recently from 4358899 to 31e1094 Compare April 26, 2023 15:24
@vercel
Copy link

vercel bot commented Apr 26, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 28, 2023 6:10pm

@k-boikov k-boikov marked this pull request as ready for review April 26, 2023 15:38
@k-boikov k-boikov force-pushed the improvement/remove-colors-refactor-plus-tests branch 2 times, most recently from 28cd383 to 310e8bc Compare April 26, 2023 16:25
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 26, 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.

@k-boikov k-boikov force-pushed the improvement/remove-colors-refactor-plus-tests branch from 310e8bc to e9fe1f4 Compare April 26, 2023 22:27
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Apr 26, 2023
@github-actions
Copy link
Contributor

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

@k-boikov k-boikov force-pushed the improvement/remove-colors-refactor-plus-tests branch from e9fe1f4 to a0b9ae9 Compare April 26, 2023 22:31
@k-boikov k-boikov force-pushed the improvement/remove-colors-refactor-plus-tests branch from a0b9ae9 to 47883ce Compare April 27, 2023 13:49
@lc0rp
Fix for ai_settings goals loaded as list(dict)
67d3fd9 
Some ai_settings formats can cause goals to load as list(dict)
not list(str)

Refactor code in utils.py to explicitly convert input type to string in
remove_color_codes() function.

- Updated remove_color_codes function to convert input argument
 to string type explicitly to avoid unexpected type errors.
- Test case added to check conversion of dict to string in
remove_color_codes function.
@lc0rp lc0rp mentioned this pull request Apr 28, 2023
Closed
5 tasks
@collijk
Copy link
Contributor

collijk commented Apr 28, 2023

Why have you moved the function from the only module in which it is used?

collijk
collijk reviewed Apr 28, 2023
View reviewed changes
tests/test_utils.py Outdated Show resolved Hide resolved
collijk
collijk suggested changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@collijk collijk left a comment

Choose a reason for hiding this comment

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

The function move needs justification. It's okay if you can explain why you're doing it (e.g. do you have another pr in the works where you need this function outside the logs module?). Otherwise it needs to move back.

The tests are great! They need parameterization though. Code changes are suggested. I recommend taking a look at the pytest docs to see more about paramerizing tests and using fixture. They are amazingly useful features for organizing tests.

@github-actions github-actions bot added size/l and removed size/m labels Apr 28, 2023
@github-actions github-actions bot added size/m and removed size/l labels Apr 28, 2023
@vercel vercel bot temporarily deployed to Preview April 28, 2023 18:10 Inactive
collijk
collijk approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@collijk collijk left a comment

Choose a reason for hiding this comment

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

Looks great!

@collijk collijk merged commit c1f1da2 into Significant-Gravitas:master Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@collijk collijk collijk approved these changes

Assignees
No one assigned
Labels
size/m testing
Projects
AutoGPT development kanban
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@k-boikov @collijk @lc0rp