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

chore: remove unused import to pass flake8 on main #5282

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

p2edwards
Copy link
Contributor

@p2edwards p2edwards commented Nov 20, 2024

πŸ‘€ Preview steps

  • πŸ”΄ [on main] recent (but not related) PR failing 'darker' flake8 linter test due to unused import β€” ❌ #5264
  • 🟒 [on PR] i think this might fix it? β€” βœ… here

@p2edwards
Copy link
Contributor Author

p2edwards commented Nov 20, 2024

I'm starting to see how this happens

Currently the darker workflow compares:

  • the pr's base commit in main
  • the pr's merge commit, the results if this PR was merged right now

So if new commits appear in main, and then you push a new commit or trigger a workflow run again, you may get darker warnings on your PR that were introduced by unrelated changes in main that aren't in your 'branch' (and possibly didn't appear in CI for those PR's either).

Ways to make the signal clearer

Option 1: Compare latest main and the merge ref

Run linter on merged result, comparing:

  • the latest HEAD of main ( ✏️ see ken muse's blog post for how to get this)
  • the pr's merge commit (main after merge)
  • Pros/cons:
    • πŸ‘ If merging your PR would introduce linter warnings in main (at time of workflow run), you'll see those warnings.
    • ❕ You might see warnings in CI that you can't see locally unless you merge main or rebase your branch
    • ❔ Stale 'success' results could still lead to a 'main' failing CI after merge. If it's been a while since the tests were run, it may be a good idea to manually re-trigger before merging

Option 2: Compare PR base and head refs

Run linter just on the PR branch (before merge), comparing:

  • the pr's base commit (${{ github.event.pull_request.base.sha }})
  • the pr's head commit ( ✏️ make actions/checkout to use ${{ github.event.pull_request.head.sha }} )
  • Pros/cons:
    • ❔ Results of tests confirm what you'd see locally
    • ❕ If the goal is to prevent surprises in main after merging, this wouldn't fully do it

Option 3: A combo of both

  • Failing on one or the other indicates an issue and a good next step
  • For tests that are cheap, this can help spot the source.
    • merge test is more important overall
    • a passing pr test with a failing merge test tells you you need to merge main

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.

1 participant