-
Notifications
You must be signed in to change notification settings - Fork 750
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
Added format script to simplify avoiding .semgrep directory #2946
Conversation
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.
LGTM
scripts/format.sh
Outdated
# Build a list of all the top-level directories in the project. | ||
for DIRECTORY in */ ; do | ||
GOGLOB="$GOGLOB ${DIRECTORY%/}" | ||
done | ||
GOGLOB="${GOGLOB/ docs/}" | ||
GOGLOB="${GOGLOB/ vendor/}" | ||
|
||
GOFMT_LINES=`gofmt -s -l $GOGLOB | tr '\\\\' '/' | wc -l | xargs` | ||
|
||
if [[ $GOFMT_LINES -ne 0 ]]; then | ||
FMT_FILES=`gofmt -s -l $GOGLOB | tr '\\\\' '/' | xargs` | ||
for FILE in $FMT_FILES; do | ||
echo "Running: gofmt -s -w $FILE" | ||
`gofmt -s -w $FILE` | ||
done | ||
fi |
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.
How about adding above code in makefile? With this change,
.phoney: format
// code....
- User can run
make format
command to rungofmt
- Also we can update
validate.sh
to runmake format
command. This will eliminate code duplication between format.sh and validate.sh files
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.
This is good point! I'll see what I can do
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.
@onkarvhanumante please see these changes ff28932
I did not put a bash script itself to the Makefile, instead it calls this script from the sh file. I think it looks cleaner and aligns with overall style of the Makefile and validate.sh script. What do you think?
GOFMT_LINES=`gofmt -s -l $GOGLOB | tr '\\\\' '/' | wc -l | xargs` | ||
|
||
if [[ $GOFMT_LINES -ne 0 ]]; then | ||
if $AUTOFMT; then |
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.
$AUTOFMT
is currently not defined in format.sh
. Script should accept $AUTOFMT
as a command-line input. If no value is provided when running the script, $AUTOFMT
will default to false. Also should update callers to pass $AUTOFMT
value from format.sh
callers
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.
Good catch, fixed!
Resolves #2925