-
-
Notifications
You must be signed in to change notification settings - Fork 12
Create tool for linting Arduino projects #1
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
Conversation
6434353 to
8b5a200
Compare
8b5a200 to
7ef7cf2
Compare
7ef7cf2 to
d84106a
Compare
I watched the whole PR and left lots of comments but I must have missed something for sure.
I've not commented some indentation issues but would love if you could fix those too.
Also I've noticed you call String() lots of times when printing or logging, I'm not completely sure but it shouldn't be necessary most of the times since you're already using the %s verb for formatting.
12c0dbc to
35115a2
Compare
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.
🚀
I watched the whole PR and left lots of comments
Thanks so much. Great suggestions!
I've not commented some indentation issues but would love if you could fix those too.
I have run task go:format on the project without any changes. However, it's possible that there are style choices that are not enforced by go fmt. I'm very much receptive to any suggestions for formatting improvements. If you could provide me with an example, I can then fix that issue and also do a survey of the code to apply the same sort of improvement comprehensively.
Also I've noticed you call String() lots of times when printing or logging, I'm not completely sure but it shouldn't be necessary most of the times since you're already using the %s verb for formatting.
There was definitely a lot of unnecessary use of String(). I believe I have removed all of them in 73d8eab and what remains is all necessary. I guess it was propagated from some early use of print() and then I just never realized it wasn't needed anymore after updating the output code.
- Put type in dedicated package to create a namespace for it - Name custom type "Type" (the package name provides the identification)
Checks should always be explicitly configured to run or not run in any check mode.
I made a dedicated function for this purpose, then forgot to use it!
This information is not normally of interest.
When a report file path is specified by the user, a JSON formatted report of the checks will be written to that path.
Since the variables already contain the value that needs to be returned, the if statement only adds unnecessary verbosity.
This would indicate a new project type was added to the tool without updating the subproject discovery code accordingly.
Previously, the more general project filter type was used, but the specific project type that was detected will be more useful information.
The term "project type" is used both to refer to the type of the project filter, which may be "all", and to the specific project type (e.g., sketch, library, platform). This can result in some ambiguity in the code. The use of "filter" in variable names that are used exclusively for project type filters may make the code easier to follow.
Consolidate the redundant logging code in these functions.
Previously, there was no provision for multiple report instances. Although the current tool design has no need for multiple instances, the code now has the versatility to allow for report demands that might arise in the future.
The change in approach in the result package resulted in some of the use of the term "report" in that package becoming less intuitive, since reports are only one use of the results.
e905cb4 to
09bbd6a
Compare
No description provided.