-
Notifications
You must be signed in to change notification settings - Fork 503
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
all: simplify gofmt script #1777
all: simplify gofmt script #1777
Conversation
@@ -2,9 +2,7 @@ | |||
set -e | |||
|
|||
printf "Running gofmt checks...\n" | |||
OUTPUT=$(ls -d */ \ | |||
| egrep -v '^vendor|^docs' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These directories were previously being ignored. Now they're being checked by gofmt
. That doesn't sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I meant to mention this in the description. vendor
no longer exists. Having the docs folder included in the scan is fine, at least today. Looking at the git history of the file and previous PRs didn't tell me why so I can only theorize on why it was included. Maybe an older version of gofmt
would error when there were no go files in it. That's what go fmt
does today, although not gofmt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Yeah it's probably something like that. Pretty sure this was here for a reason in the past, but if it works now, that's great.
@@ -2,9 +2,7 @@ | |||
set -e | |||
|
|||
printf "Running gofmt checks...\n" | |||
OUTPUT=$(ls -d */ \ | |||
| egrep -v '^vendor|^docs' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Yeah it's probably something like that. Pretty sure this was here for a reason in the past, but if it works now, that's great.
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
This PR adds tests for the most critical parts of the new functionality or fixes.I've updated any docs (developer docs,.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
I've updated the relevant CHANGELOG (here for Horizon) ifneeded with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
Summary
Remove the ls xargs dance from the gofmt and just get gofmt to run it's checks over everything of what's different.
Goal and scope
I was looking at copying this script to another repository and noticed that the dance with excluding certain folders and piping to xargs to run in parallel is really not necessary.
I'm not making this change to improve the performance of this, because I don't think it's meaningful right here, but just mentioning this for completeness that when I run the script as it is now sharing across 4 processes it was actually marginally slower than just running gofmt once like this change does. So this change isn't introducing a slowdown in CI runtime.
Summary of changes
Known limitations & issues
N/A
What shouldn't be reviewed
N/A