Skip to content

have zig fmt notice a special comment that tells it to skip a source file #1030

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

Closed
andrewrk opened this issue May 30, 2018 · 5 comments
Closed
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

Currently zig has 3 files that we want to always skip formatting:

  • std/special/compiler_rt/udivmodti4_test.zig
    • it's 1.8MiB so we want to only update the syntax when necessary
  • std/special/compiler_rt/udivmoddi4_test.zig
    • it's 9.8MiB so we want to only update the syntax when necessary
  • test/cases/syntax.zig
    • it's testing parsing edge cases, so running zig fmt would defeat the purpose of the tests

Here's my idea:

// zig fmt: skip

... normal code ...

If the first token in the file is a line comment that has the contents zig fmt: skip then zig fmt skips the file.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label May 30, 2018
@andrewrk andrewrk added this to the 0.3.0 milestone May 30, 2018
@ghost
Copy link

ghost commented May 30, 2018

// clang-format off
Some code
// clang-format on

Works well for cpp, maybe this would be a good fit for zig fmt as well.

Just
// zig-format off
Would disable it for the whole file

Only downside I can think of is that people use it more frequently and code does not get formatted uniquely everywhere.

@andrewrk andrewrk modified the milestones: 0.3.0, 0.4.0 Jun 1, 2018
@bronze1man
Copy link

bronze1man commented Jun 2, 2018

What about the git merge conflicts problem?
it should be very serious to the 9.8MiB file.

go fmt in the golang can create a lot of git merge conflicts.
So I do not use it any more.

Our team have an rule about format code with ide or tool:

  • Only format the code your just writen.
  • Do not format the old code.
  • Do not format all the code in the project.

@ghost
Copy link

ghost commented Jun 2, 2018

What about the git merge conflicts problem?

https://git-scm.com/docs/merge-strategies

and #1037 (comment)

Do not format the old code.

formatting is idempotent, applying formatting to old code over and over does thus not change anything

Do not format all the code in the project.

for special cases there will be #1030 (comment) zig-fmt off

@bronze1man
Copy link

bronze1man commented Jun 2, 2018

It is strange that go fmt actually increase the number of git conflicts in the long term in my experience.

If you add a long field into a golang struct in one place, and go fmt it. All the line of that struct changed. So it will conflicts when another guy add a field into this struct in another place.
If you do not use the go fmt in that case. The code looks ugly, but it only add 1 line.Other line of struct do not change. So it will not conflicts in above example.

May be there is a design bug in go fmt. Or I should not add two different fields into a struct in two git branch and merge them.

I know that the design bug in go fmt can not be fixed because it will break lots of the exist code.
It maybe avoid in the zig fmt, so we can use it before build.

@andrewrk
Copy link
Member Author

andrewrk commented Jun 2, 2018

It is strange that go fmt actually increase the number of git conflicts in the long term in my experience.

One reason for this is that go fmt aligns everything. Meanwhile zig fmt does not align everything, so it is more diff friendly.

@andrewrk andrewrk modified the milestones: 0.4.0, 0.3.0 Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants