Skip to content

Conversation

@sir-sigurd
Copy link
Member

@sir-sigurd sir-sigurd commented Oct 15, 2025

Greptile Overview

Updated On: 2025-10-15 13:11:15 UTC

Summary

Enabled E501 (line-too-long) enforcement in ruff by removing it from the ignore list, ensuring all Python code adheres to the 119-character line limit.

  • Removed E501 from ruff.toml ignore list to enforce line length checks
  • Fixed the one existing violation in athena.py by moving an inline comment to a separate line

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • The changes are purely cosmetic - enabling a linter rule and fixing the single violation by reformatting a comment. No functional code was modified, and the change improves code quality enforcement going forward.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
ruff.toml 5/5 Removed E501 (line-too-long) from ignored rules to enforce line length limit
py-shared/src/quilt_shared/athena.py 5/5 Split overly long comment line to comply with 119 character limit

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant Ruff as Ruff Linter
    participant Code as Codebase
    
    Dev->>Ruff: Remove E501 from ignore list
    Note over Ruff: E501 (line-too-long) now enforced
    Ruff->>Code: Scan for lines > 119 chars
    Code-->>Ruff: Found line 113 in athena.py (122 chars)
    Dev->>Code: Split comment to separate lines
    Note over Code: Line 113: comment moved<br/>Line 114: code remains
    Ruff->>Code: Re-scan all files
    Code-->>Ruff: All lines comply with 119 char limit
    Ruff-->>Dev: Linting passes ✓
Loading

@sir-sigurd sir-sigurd requested a review from nl0 October 15, 2025 13:09
@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.37%. Comparing base (d92a712) to head (929d151).

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #4605    +/-   ##
========================================
  Coverage   39.37%   39.37%            
========================================
  Files         878      878            
  Lines       37664    37664            
  Branches     6145     5889   -256     
========================================
  Hits        14829    14829            
- Misses      21579    22317   +738     
+ Partials     1256      518   -738     
Flag Coverage Δ
api-python 91.67% <ø> (ø)
catalog 19.96% <ø> (ø)
lambda 92.60% <ø> (ø)
py-shared 97.18% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile


# Ignore rules that were disabled in pylint or would cause too many changes
ignore = [
"E501", # line-too-long (already handled by line-length setting)
Copy link
Member

Choose a reason for hiding this comment

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

i don't think we should do that, because that applies two similar, but not exactly the same checks. we already handle that by the formatting task, so if anything has to be reformatted due to line length, it will cause an error in the formatting ci job

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think we should do that, because that applies two similar, but not exactly the same checks.

I don't understand that

Copy link
Member

Choose a reason for hiding this comment

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

i mean ruff formatting logic is not that strict about the line lengths compared to the linting logic,
e.g. it doesn't complain on that comment line you changed in this PR, otherwise the formatting job would fail
oh i see we don't have a repo-wide formatting check in CI.
but anyways, i don't think we want two CI jobs checking one thing -- it's enough to enforce this in the formatting job (which is commented-out currently)

Copy link
Member

Choose a reason for hiding this comment

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

has ruff format done that change, btw?

Copy link
Member Author

@sir-sigurd sir-sigurd Oct 15, 2025

Choose a reason for hiding this comment

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

has ruff format done that change, btw?

no

but anyways, i don't think we want two CI jobs checking one thing

I don't see any problems with it and I don't want to have long lines with comments like I fixed in this PR (or with long string which AFAICT ruff won't reformat as well)

we have both black and pycodestyle in enterprise and I don't think it caused any issues
also prior to using ruff we had E501-like check in this repo

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