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 support for JSON output writing #14

Merged
merged 7 commits into from
Oct 31, 2022
Merged

Conversation

davidkroell
Copy link
Contributor

Adding a new flag -format to set the output writer formatting. The default behavior keeps the same, as text is the default format.

Adding this functionality required to refactor the code a bit. Logic for filtering (top/over) and writing is now moved to the gocognit package.

Also updated to go1.18

Copy link
Owner

@uudashr uudashr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you test the behaviors of the command line? We need to know whether the behavior is backward compatible or not. Since there might be some people utilized the command line for automation

over = flag.Int("over", defaultValueIndicator, "show functions with complexity > N only")
top = flag.Int("top", defaultValueIndicator, "show the top N most complex functions only")
avg = flag.Bool("avg", false, "show the average complexity")
format = flag.String("format", "text", fmt.Sprintf("which format to use, supported formats: %v", supportedFormats))
Copy link
Owner

@uudashr uudashr Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think better to use textFormat constant value instead of "text"? It yield the same value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ofc, haven't seen that - thanks

if err != nil {
panic(err)
}

return len(sortedStats)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should returned the filtered stats? Initially the return value used on main() to indicate wether we have stats written or not.

With your changes, the exit code not only decided in main() function, but also in writeStats(..). Do you have any idea to make it better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've now refactored it a bit. Nevertheless this should be a very rare case.

@uudashr
Copy link
Owner

uudashr commented Oct 5, 2022

I've made some changes on master.
The Changes: log.Fatal(..) and os.Exit(..) only happen in main() function

Could you rebase from master, it might help to ensure the behavior of the command line is backward compatible

@davidkroell
Copy link
Contributor Author

I've refactored it and also did the rebase. Please review again

break
default:
fmt.Printf("Format '%s' is not valid, use a supported format %v", *format, supportedFormats)
os.Exit(1)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put all the exit (os.Exit or log.Fatal) on the func main(). What if we return an error?

formatter.go Show resolved Hide resolved
@uudashr
Copy link
Owner

uudashr commented Oct 13, 2022

The test is fail on 1.16. Do you have any specific reason why you upgrade the go version to 1.18?

@uudashr
Copy link
Owner

uudashr commented Oct 13, 2022

Change it back to Go 1.16 should make it work

@uudashr
Copy link
Owner

uudashr commented Oct 21, 2022

@davidkroell May I know why you need this json format?

@davidkroell
Copy link
Contributor Author

I need this because I'd like to build a suite of quality checkers like this and generate reports based on these results and therefore json is an easy format to work with.
Thanks for merging!

@uudashr uudashr merged commit 5c1963c into uudashr:master Oct 31, 2022
@uudashr
Copy link
Owner

uudashr commented Jan 5, 2023

Hi @davidkroell, do you really need feature for for non-pretty JSON? Or any kind of JSON output is fine?

@davidkroell
Copy link
Contributor Author

Hi @uudashr any JSON output is fine.

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.

2 participants