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
cmd: Refactor main() through helpers. #940
Comments
Generally I agree that the
I don't really agree with this reason in itself; while it certainly helps to break out related pieces of logic into their own functions (since that's what functions are for, after all), doing this for the sake of "keeping functions short" usually results in unnecessarily deep call stacks and/or more overly-long helper functions, or code that's so disjoint it impedes clarity.
To achieve the "shape" you're talking about implies putting nearly all common state in a giant struct or making it all global, or having functions with lots of parameters and return values. I'd be OK with putting the parsed flags themselves in a struct (since that's also a way for us to sneak in features such as configuring via file (#101), but I'm curious if there is additional common state that we'd end up passing everywhere?
Just curious: for the the pricing flags from that discussion, is there anything prior to that check which really ought to be
It's customary for (Unix) programs to return error codes based on the type of failure; this gives external tools some more information about why a program may have failed. I've thought about using this to help make our CLI tests more specific, but that's a separate issue. |
To clarify, I'm talking explicitly about the main function, not other functions. In my opinion, the main function should be used to fire up the program and nothing else. Perhaps I should remove the numbering in the original post, as I did not want to imply that this is the most important reason. The most important reason should be maintainability and being able to do proper error handling (as was the conclusion after the discussion I had with Yondon), the fact that it becomes short is just a positive side effect I guess. In my opinion Cobra generated CLI programs are a great example of this.
I was personally thinking of making those global rather than a wrapping struct. Using following syntax:
Altough these don't need to be pointers if we dereference when calling
Yeah on second consideration when skimming through the main function these could be better placed but would need some duplication of e.g. offchain/onchain checks and what type of node is being ran. The thing is that the checks imposed on the pricing flags actually use In other places in the main we are still using
Oh yes, that would be nice. I didn't think of that. We could scope that in, considering both issues aren't priorities but just code optimisations down the line. |
+1 for refactoring the |
Is your feature request related to a problem? Please describe.
We should prevent the main function from terminating before deferred functions can be executed. Currently we are mainly using
glog.Fatal
which is equivalent to callingos.Exit(1)
, this exits the program not allowing any deferred functions to run.In some statements
panic
is used, which executes deferred functions after throwing but it is not idiomatic to panic in such contexts.Idiomatically the main function should be kept short and purely used for startup of the program. Startup of services should be initiated through helpers functions.
We could refactor the cluttered main to simply
glog.Error + return
instead ofglog.Fatal / os.Exit(1)
but this makes testing the returned error codes intest_args.sh
impossibleAdditionally refactoring main makes it easier to maintain the main function and add functionality for the startup of services as needed.
Describe the solution you'd like
Extract the startup of various services into helpers. Such as
parseFlags
startDBservice
startEthClient
setPriceInfo
startMediaServer
initS3storage
startLivepeerNode
All error statements should be refactored to match the following pattern
If the helper returns it's deferred functions will execute. We can then safely os.Exit() in the main after encountering an error
eg.
This is the same pattern used by Cobra:
eg.
Additional context
See this discussion in #933
The text was updated successfully, but these errors were encountered: