Skip to content

Format project using Air #291

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

VisruthSK
Copy link
Collaborator

Use Posit's new opinionated formatter, Air to review pull requests to ensure they conform to the style as well as format the entire project once.

What do you think? @jgabry @avehtari

@jgabry
Copy link
Member

jgabry commented Jun 12, 2025

In general I'm in favor of something like this. My only concern is that in their GitHub readme they say:

Air is currently in beta. Expect breaking changes both in the API and in formatting results.

So it seems like it's not stable yet. If they're still making changes to the formatting results will we need to reformat the entire project every time they do that? If we don't then won't PRs that get auto-formatted using their changes be formatted differently than the rest of the project?

@jgabry jgabry closed this Jun 12, 2025
@jgabry jgabry reopened this Jun 12, 2025
@jgabry
Copy link
Member

jgabry commented Jun 12, 2025

(Oops accidentally closed the PR, I meant to leave it open to continue the discussion. Reopening now.)

@VisruthSK
Copy link
Collaborator Author

I think formatting is simple enough to add/maintain and low-risk enough that breaking changes don't matter as much--worst case one could just remove the action and there's no cost.

I think the action reformats the entire project run: air format . --check. It's rather fast; when I ran it on loo the whole thing was done in under a second or two. So I think if the formatting rules changed, new PRs would reformat the entire project anyway, so the formatting would be consistent throughout.

@jgabry
Copy link
Member

jgabry commented Jun 13, 2025

Sorry I think I wasn't clear about my concerns. Here's hopefully a better summary of what makes me a bit concerned to use this while it's in beta (and the entire project may be reformatted multiple times). All of these issues are less of a problem if the formatting rules are stable, so I definitely think this is a good idea at some point.

Here's why I'm hesitant while it's in beta:

  • Frequent reformatting may introduce large changes to many files. This makes it difficult to track actual code changes, bugs, or feature additions, because the commit history becomes cluttered with formatting commits.
  • Ongoing development (feature branches, bugfixes) will be more likely to conflict with formatting changes on the main branch, so we may have more merge conflicts to sort out, which can be tedious.
  • Tools like git blame become less useful because lines of code may change multiple times for non-functional reasons.
  • Reviewing meaningful changes in pull requests becomes harder, since diffs may be dominated by formatting noise.

Of course they may not make many changes to the formatting rules while in beta, but it's hard to know and they do say to expect changes.

@VisruthSK
Copy link
Collaborator Author

Ah that makes sense, thank you for clarifying. I didn't think of the diff clutter, but now that you mention it, that does seem quite distracting and not worth it for now.

Do you think it'd make sense to push off formatting PRs till Air is stable, but format the entire project on every release? This would have the problem that you mentioned of odd formatting in between releases though. Or do you think it makes more sense to just wait till Air is stable before using it at all?

@jgabry
Copy link
Member

jgabry commented Jun 13, 2025

I would lean towards waiting until it's stable to use it, but I'll think more about it. And maybe they will do a 1.0 release soon. We can definitely leave this PR open in the meantime. Or maybe convert it to a draft PR if it's possible to convert after opening it (or close and reopen as a draft). I definitely think this would be a good thing to do when it's ready.

@VisruthSK VisruthSK marked this pull request as draft June 14, 2025 03:59
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