Reduce boilerplate code by rationalization of context data into a struct #134
Conversation
For now it is used in conjunction with the old map[string]string, but it will be slowly populated with all the fields currently used in the map in the next commits. When the migration will be completed the old map will be removed. Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Still missing the refactoring of main.go that will be done in one of the next commits Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
I don't know if logger should be part of the Context, anyway I kept it in the structure to maximize compatibility. Default logging level has been lowered from 5 to 0, so the initialization step could be completely removed. Logging subroutines have been adjusted accordingly. Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Removed also utils.AddStringsToStringsSet(..) because passing through maps for removing duplicated elements doesn't guarantee to preserve the list order. Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
fdafc4b to
e10fcc8
Compare
| @@ -249,8 +248,7 @@ func main() { | |||
| _, err := os.Stat(buildPath) | |||
| if err != nil { | |||
| fmt.Fprintln(os.Stderr, err) | |||
| defer os.Exit(1) | |||
| return | |||
| os.Exit(1) | |||
There was a problem hiding this comment.
Isn't the defer useful here? IIRC deferring the exit was useful to make sure other defer and error handlers run, right? Or does that not apply here?
There was a problem hiding this comment.
Oh good catch, that's a nice one that I forget to comment on the PR.
I kept this change in a single commit so it can be easily reverted in case.
IIRC deferring the exit was useful to make sure other defer and error handlers run
what you said applies for deferred actions created in the current scope (i.e. the current running function and its callers), but since the main function, basically, does only command line parsing all the defers are instantiated inside "Commands" and when the control returns back to the main function all the internal defers are already out of scope and executed. Since the main function doesn't use defer I opted to skip the defer os.Exit construct.
The drawback is that new defers may be added in the main function in the future and it would be more future-proof to keep the defer os.Exit construct instead. Personally I prefer the "classic" behavior of the "exit" function, where the execution of the program is terminated immediately.
There was a problem hiding this comment.
Another approach may be to make the main function just call sub-functions and convert the result into an exit code, something like:
func main() {
if err := parseCommandLine(); err != nil {
exitWithError(err);
}
if err := runCommands(); err != nil {
exitWithError(err);
}
os.Exit(0)
}
func exitWithError(err error) {
var exitCode int
// [...convert error into an exitCode...]
os.Exit(exitCode)
}There was a problem hiding this comment.
I guess you could even use a more simple approach where the effective main function can just return an exit code, just like we know from C:
func main() {
os.Exit(realMain())
}
And then the realMain() function just does what it needs and returns an exit code?
There was a problem hiding this comment.
Yup that's another way too, BTW my preference is to handle everything with golang's error until the very end and use the main function to convert it into an error code. BTW I guess we can work this out after merging this PR :-)
|
I had a more thorough look over this PR, and overall it looks good. What a gargantuan amount of work, though, kudos for seeing it through :-) However, I can't help but think we should be reducing the context object at some point, since effectively we're now just using global variables (hidden in a context struct) to pass function arguments and results around (very much commodore basic-style). We will need some other way to handle passing data around then, of course. Anyway, something to think about later, I think. |
| if librariesFolders, err := toSliceOfUnquoted(librariesFoldersFlag); err != nil { | ||
| printCompleteError(err) | ||
| } else if len(librariesFolders) > 0 { | ||
| ctx.LibrariesFolders = librariesFolders |
There was a problem hiding this comment.
There is a bug here, this should be ctx.OtherLibrariesFolder instead. ctx.LibrariesFolders is used by LoadLibraries to store the final complete and sorted list of library folders.
There was a problem hiding this comment.
You're right, great catch! I'm fixing it...
main.go has been moved into module arduino.cc/arduino-builder Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Some helper "Command" now accepts directly the address of the target string to work on instead of the key of the context map that contains the string. Those "arguments" are, I guess, legacy from old preprocessing and library discovery and IMHO they can be removed with a bit of refactoring. Anyway, I've intentionally limited the scope of this commit and left further optimization for the future. Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Probably a better name for the Packages structure is Hardware, something to see later. Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
It seems legacy, shall we remove it? eventually may turn out to be useful in the future? Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Probably these structures should be renamed to something more meaningful Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
This commit concludes the refactoring. Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
e10fcc8 to
e6f25db
Compare
|
✅ Build completed. ⬇️ Build URL: ℹ️ To test this build:
|
|
This PR works like a charm, and now it's really a pleasure to write new code without getting mad 😄 |
related to #134 Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
This pull-request aims to dramatically increase readability of the arduino-builder source code by removing a lot of boilerplate code. This change set involves a large part of the builder.
The rationale behind this PR has been partially discussed here: #87 (comment)
Basically I'm trying to use an explicit golang
structinstead of a map-of-strings-to-an-empty-interfacemap[string]interface{}. Doing so brings the following advantagesconstants.CTX_....because the compiler already checks correctness of the field name, for example:interface{}value to the correct type as we did before:boolmembers tofalse, we could remove this initialization snippet: