gittuf / gittuf

A security layer for Git repositories
https://gittuf.dev
Apache License 2.0
439 stars 28 forks source link

Made make fmt faster #401

Closed neilnaveen closed 1 month ago

neilnaveen commented 1 month ago
adityasaky commented 1 month ago

Do we have a bottleneck here? time tells me this takes about 0.200s (admittedly in a codebase that's already formatted). I'm wary of replacing it with something that's decidedly harder to read and understand if there isn't a significant gain.

patzielinski commented 1 month ago

This seems cool, but I agree with the above. Maybe we can have something like make fmt-fast if we do in fact want to include a faster version of make fmt?

adityasaky commented 1 month ago

I'd like to see some examples showing a significant improvement in time for that, I'm rather opposed to providing replacements / alternatives to the simple, succinct go fmt ./... (that all Go developers likely recognize), when said alternative has a lot more to grok. Even if we have a 50% improvement, if in absolute terms we're shaving off 0.1s, I don't think there's real value yet. Do we know a case when go fmt ./... is really slow?

neilnaveen commented 1 month ago

I was thinking of how in each PR, fmting takes around a minute, and if it was faster, we would be able to address those lint errors faster, just for convenience.

I got this linting code from https://github.com/guacsec/guac/blob/4dc7c94c4aa29eda038e1194342c324ad5962861/.github/workflows/ci.yaml#L86-L103, and https://github.com/guacsec/guac/blob/4dc7c94c4aa29eda038e1194342c324ad5962861/Makefile#L61

adityasaky commented 1 month ago

Several things: a) golangci-lint on actions isn't running make fmt b) It should be faster now with #400 c) Since golangci-lint isn't just go fmt under the hood, I suggest perhaps installing the linter locally and running it for faster feedback? :smile:

I also noticed that guac has golangci-lint in the CI as well, as a separate step. I'm going to close this PR now, thank you for the suggestion!