Skip to content

Suppress animation in CI contexts Fixes #702#703

Merged
ben-edna merged 1 commit intomainfrom
spoorcc/issue702
Mar 21, 2025
Merged

Suppress animation in CI contexts Fixes #702#703
ben-edna merged 1 commit intomainfrom
spoorcc/issue702

Conversation

@spoorcc
Copy link
Copy Markdown
Contributor

@spoorcc spoorcc commented Mar 21, 2025

Description by Korbit AI

What change is being made?

Implement suppression of animation when running in Continuous Integration (CI) environments.

Why are these changes being made?

These changes resolve issue #702 by adding logic to check the CI environment variable and disable animations accordingly. This modification improves usability by preventing unnecessary animations in non-interactive environments like CI, which does not benefit from visual feedback. The changes keep interactive session output clean and focused on essential information.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

Copy link
Copy Markdown

@korbit-ai korbit-ai Bot left a comment

Choose a reason for hiding this comment

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

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Fix Detected
Functionality Incomplete CI Environment Detection ▹ view
Files scanned
File Path Reviewed
dfetch/project/vcs.py

Explore our documentation to understand the languages and file types we support and the files we ignore.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

  • You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
  • You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
  • Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
  • On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
  • Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

  • Check out our docs on how you can make Korbit work best for you and your team.
  • Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings
Setting Value
Review Schedule Automatic excluding drafts
Max Issue Count 10
Automatic PR Descriptions
Issue Categories
Category Enabled
Documentation
Logging
Error Handling
Readability
Design
Performance
Security
Functionality

Feedback and Support

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

Comment thread dfetch/project/vcs.py Outdated
def _running_in_ci() -> bool:
"""Are we running in CI."""
ci_env_var = os.getenv('CI', None)
return ci_env_var and (ci_env_var.lower().startswith("t") or ci_env_var == "1")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Incomplete CI Environment Detection category Functionality

Tell me more
What is the issue?

The CI environment detection logic only accepts 'true', 't' (case-insensitive), or '1' as valid values, but misses common CI environment variations like 'TRUE', 'yes', 'Y', etc.

Why this matters

CI systems use various truthy values to set environment variables. The current implementation might fail to detect some CI environments, causing animations to run when they shouldn't be suppressed.

Suggested change ∙ Feature Preview

Expand the CI environment detection to include more common truthy values:

@staticmethod
def _running_in_ci() -> bool:
    """Are we running in CI."""
    ci_env_var = os.getenv('CI', '').lower()
    truthy_values = ('true', 't', '1', 'yes', 'y')
    return any(ci_env_var.startswith(val) for val in truthy_values)
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could also check the first char if it is t, y or 1

@ben-edna ben-edna merged commit 7650c2b into main Mar 21, 2025
27 checks passed
@ben-edna ben-edna deleted the spoorcc/issue702 branch March 21, 2025 20:41
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.

2 participants