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: str(ENUM) should just stringify the slug #999

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

tushar-composio
Copy link
Collaborator

@tushar-composio tushar-composio commented Dec 12, 2024

Important

Add __str__ method to Enum class to return slug, with corresponding test updates.

  • Behavior:
    • Add __str__ method to Enum class in enum.py to return self.slug.
  • Tests:
    • Add test in test_enum.py to assert str(App.GITHUB) == "GITHUB".
    • Update test in test_toolset.py to expect ('GITHUB', ...) instead of ('App.GITHUB', ...).

This description was created by Ellipsis for f6f9152. It will automatically update as commits are pushed.

Copy link

vercel bot commented Dec 12, 2024

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

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2024 8:48am

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 477cd81 in 10 seconds

More details
  • Looked at 27 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tests/test_client/test_enum.py:120
  • Draft comment:
    Consider adding similar __str__ method test cases for other enum types like Action, Trigger, and Tag to ensure consistency and correctness across all enum types.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The __str__ method implementation in the Enum class is correct and aligns with the test case added in test_app_enum. However, the test case for __str__ is only added for App.GITHUB. It would be beneficial to add similar test cases for other enum types to ensure consistency and correctness across the board.

Workflow ID: wflow_HkMpmIGZ14IqFqZ0


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@shreysingla11
Copy link
Collaborator

Code Review Summary

The changes look good overall. The PR adds a proper string representation to the Enum class which is a useful addition. The implementation is simple, correct, and consistent with Python's string representation conventions.

Strengths:

  • Simple and focused implementation
  • Consistent with Python's string representation conventions
  • Maintains backward compatibility
  • Includes test coverage

Suggestions for improvement:

  1. Add docstring to the __str__ method for better documentation
  2. Add more test cases to cover different scenarios

The code quality is good, and with the suggested improvements, it would be even better. The changes are ready to be merged after addressing the minor suggestions.

@tushar-composio tushar-composio changed the title Hotfix enum str fix: str(ENUM) should just stringify the slug Dec 12, 2024
@tushar-composio tushar-composio merged commit 259db26 into master Dec 12, 2024
9 checks passed
@tushar-composio tushar-composio deleted the hotfix-enum-str branch December 12, 2024 08:51
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Incremental review on f6f9152 in 5 minutes and 29 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tests/test_tools/test_toolset.py:123
  • Draft comment:
    The change from App.GITHUB to GITHUB in the test is correct and aligns with the new __str__ method implementation for the Enum class, which returns the slug.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in the test is consistent with the PR description, which aims to change the string representation of the enum to its slug. The test now correctly expects 'GITHUB' instead of 'App.GITHUB'.

Workflow ID: wflow_Mkkv3ntA7FidK4Te


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants