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: avoid duplicate input in basic auth #894

Merged
merged 1 commit into from
Nov 23, 2024
Merged

fix: avoid duplicate input in basic auth #894

merged 1 commit into from
Nov 23, 2024

Conversation

tushar-composio
Copy link
Contributor

@tushar-composio tushar-composio commented Nov 23, 2024

_collect_input_fields takes user input, we should only be calling it once.


Important

Fix duplicate input collection in _handle_basic_auth by reusing auth_config in add.py.

  • Behavior:
    • In _handle_basic_auth in add.py, _collect_input_fields is now called once and its result is reused for auth_config and field_inputs.
  • Functions:
    • _handle_basic_auth now stores the result of _collect_input_fields in auth_config and reuses it, preventing duplicate input collection.

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

Copy link

vercel bot commented Nov 23, 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 Nov 23, 2024 7:06am

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 db48d78 in 16 seconds

More details
  • Looked at 32 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/composio/cli/add.py:328
  • Draft comment:
    The docstring for _collect_input_fields is incomplete. Consider providing a more detailed description of the function's purpose and parameters.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR correctly refactors the code to avoid duplicate calls to _collect_input_fields. However, the docstring for _collect_input_fields is incomplete and should be improved for clarity.

Workflow ID: wflow_iG6x8XuBbTWMirbB


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

@shreysingla11
Copy link
Collaborator

Code Review Summary

Changes Overview

  • ✅ Fixed duplicate user input prompts in basic authentication flow
  • ✅ Improved code efficiency by storing and reusing collected input
  • ✅ Better user experience by asking for authentication information only once

Code Quality: 8/10

Strengths:

  • Clean and focused changes
  • Follows DRY principles
  • Improves user experience
  • Maintains code readability

Suggestions for Future:

  1. Enhance docstrings for better code documentation
  2. Consider adding more robust input validation
  3. Make type hints more specific in related functions

Overall, this is a well-implemented fix that solves a real user experience issue. The changes are minimal, focused, and safe to merge. 👍

Copy link

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-11985143528/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-11985143528/html-report/report.html

@sohamganatra sohamganatra merged commit 624accf into master Nov 23, 2024
9 checks passed
@sohamganatra sohamganatra deleted the ENG-2537 branch November 23, 2024 07:47
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