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

✨ Add formatting #24

Merged
merged 13 commits into from
Nov 15, 2023
Merged

✨ Add formatting #24

merged 13 commits into from
Nov 15, 2023

Conversation

s-weigand
Copy link

@s-weigand s-weigand commented Nov 11, 2023

Hey there thanks for the nice extension was working on a CODEOWNERS file the other day and having syntax syntax highlighting and auto-complete made it a lot nicer experience. ❤

The thing that I missed was formatting and this PR tries to fix that.

The basic idea is that the code owners starting position gets aligned in one visual column with a user-specified offset from the longest file pattern part of all non comment out lines.

Before

*               @foo
  /some-path @bar
another-path/but-loooooooooonger                                 @baz

# a comment

    # an indented comment
/docs/**/*.md                     @foo @bar
# /docs/**/*.rst                     @foo @bar

After

*                                    @foo
  /some-path                         @bar
another-path/but-loooooooooonger     @baz

# a comment

    # an indented comment
/docs/**/*.md                        @foo @bar
# /docs/**/*.rst                     @foo @bar

To make cross-platform testing (and contributing) easier I removed the shell scripts and made the npm scripts instead, which also removed the need to specify the path inside of the dist files in the node_modules, hope that is ok.

@s-weigand
Copy link
Author

A huge part of the test framework setup (and choices) comes from the yo template recommended in the getting started guide

@chdsbd
Copy link
Owner

chdsbd commented Nov 12, 2023

Hey @s-weigand, thanks for the PR!

I think formatting would be a nice addition to this project.

I think aligning all usernames at the same level might create a large blast radiuses when editing a codeowners file.

At work we have a pretty large codeowners files, usually grouped by team, so I'm not sure what the rules should be.

These are a couple large example files I could find that are public

https://github.com/getsentry/sentry/blob/e63933d026146d846b9c0462430dab734438ee1b/.github/CODEOWNERS
https://github.com/vercel/next.js/blob/022cb256409b054a51a641928a6b93650137e494/.github/CODEOWNERS

@s-weigand
Copy link
Author

Hey @chdsbd

Thanks for the feedback and looking up those examples.

Looking at those examples I fully understand your concerns, but I also see that they would profit from some autoformatting 😅

Having half of the file you work on rewritten just because you edited one line and an extension you use updated would be a pretty bad UX.

How about for now just setting the default for github-code-owners.format.enabled to false so it is opt-in instead of opt-out?

I guess ideally there should be some (possibly user-defined) start-group and/or end-group syntax so that only lines in the group are affected.

I also might factor out the formatting code into a separate npm package (didn't find an existing one in my quick search) so the formatting can be enforced with something like pre-commit and is less editor-specific.

@chdsbd
Copy link
Owner

chdsbd commented Nov 15, 2023

Hey @s-weigand, I think you make some great points. This change is a great improvement for the codebase and we can always add more options in the future for controlling formatting.

I'll merge this with the default disabled

Thanks again for all the effort you put into this PR!

@chdsbd chdsbd enabled auto-merge (squash) November 15, 2023 15:16
@chdsbd chdsbd merged commit c5ad1ba into chdsbd:master Nov 15, 2023
8 checks passed
@s-weigand s-weigand deleted the add-formatting branch November 15, 2023 20:58
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.

None yet

2 participants