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

feat: support auto n to skip n confirmations #346

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

0xbrayo
Copy link
Collaborator

@0xbrayo 0xbrayo commented Dec 17, 2024

closes #332

Important

Adds limited confirmation skips to ask_execute() using auto_skip_count in ask_execute.py.

  • Behavior:
    • Introduces auto_skip_count in ask_execute.py to limit the number of confirmation skips.
    • Modifies ask_execute() to decrement auto_skip_count when skipping confirmation.
    • Uses regex to parse "auto" command with optional skip count in ask_execute().
  • Misc:
    • Imports re module in ask_execute.py for regex operations.

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

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.

❌ Changes requested. Reviewed everything up to c035a16 in 30 seconds

More details
  • Looked at 52 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_4fiilZwktGdAXVYS


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

if answer == "auto":
return (override_auto := True)
# secret option to skip asking for a number of times
match = re.match(r'(auto)(?: (\d+))?', answer)
Copy link
Contributor

Choose a reason for hiding this comment

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

The regex pattern should be anchored to the start of the string to avoid unexpected matches. Use ^ at the beginning of the pattern.

Suggested change
match = re.match(r'(auto)(?: (\d+))?', answer)
match = re.match(r'^(auto)(?: (\d+))?', answer)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't think it's necessary really. user might type a space before auto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also compiled the regex

@0xbrayo
Copy link
Collaborator Author

0xbrayo commented Dec 17, 2024

closes #332

Comment on lines 34 to 35
#Compiled regex pattern
pattern = re.compile(r'(auto)(?: (\d+))?')
Copy link
Owner

Choose a reason for hiding this comment

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

Call this something like re_auto, I don't think we need to compile it, at least not at import-time.

@ErikBjare
Copy link
Owner

Nice!

@ErikBjare ErikBjare merged commit 7d2e9e7 into ErikBjare:master Dec 19, 2024
5 of 7 checks passed
@ErikBjare ErikBjare changed the title feat: limited number of confirmation skips feat: support auto n to skip n confirmations Dec 19, 2024
@ErikBjare ErikBjare changed the title feat: support auto n to skip n confirmations feat: support auto n to skip n confirmations Dec 19, 2024
@0xbrayo
Copy link
Collaborator Author

0xbrayo commented Dec 19, 2024 via email

@0xbrayo
Copy link
Collaborator Author

0xbrayo commented Dec 19, 2024 via email

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.

Option to allow for multiple confirms, (but not inifinite)
2 participants