Closed SaiedKazemi closed 2 years ago
coveralls
commented
2 years ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| caller.go | 14 | 18 | 77.78% | ||
| <!-- | Total: | 14 | 18 | 77.78% | --> |
| Totals | |
|---|---|
| Change from base Build 388: | -14.8% |
| Covered Lines: | 29 |
| Relevant Lines: | 35 |
SaiedKazemi
commented
2 years ago Sounds good, as I'm interested in discussing the subject of default value logic interactively with you. I will email you the list of guides/articles I looked at when deciding an approach that would best fit for our use case.
SaiedKazemi
commented
2 years ago caller.go, line 33 at r1 (raw file):
Out of curiosity, why remove `rtx.Must`? This was designed to facilitate unit testing of main and other scenarios where error handling is otherwise fatal. What was clear in one line is now three lines. What is gained by this change?
Because our internal guideline on "Must functions" says that "They should not be used when ordinary error handling is possible". It's an internal link so I will share it with you separately. The guideline aside, personally, I find Go's common error checking pattern more readable because, to understand what rtx.Must() does, the reader has to learn the rtx package.
SaiedKazemi
commented
2 years ago hopannotation/hopannotation.go, line 106 at r1 (raw file):
I associate default value conditions like this with common practice in C++. I think of this as an anti-pattern for Go. Default value logic should be handled by the caller, back up to `main`. I would call this an anti-pattern because why is only this parameter special? Not in this case, but in general? Why not have the caller specify the value it intends to use? If the value is invalid, fair enough, return an error or nil. But, the caller should provide explicit values. No?
As mentioned earlier, I am interested in an interactive discussion and agree that there may be a better way to handle defaults without introducing complexity in the API.
Whether this is an anti-patter in Go, I don't have a strong opinion. Perhaps it is but please note that some standard library functions like, ioutil.TEmpDir() and module.DefaultVersion(), check for the empty string in their arguments and use default values -- similar to our use case here.
SaiedKazemi
commented
2 years ago ipcache/ipcache.go, line 54 at r1 (raw file):
Please have the caller provide the correct value. Or, perhaps, if you prefer, eliminate the parameter so that the only source is the flag.
Per previous comments, let's discuss.
SaiedKazemi
commented
2 years ago triggertrace/triggertrace.go, line 1 at r1 (raw file):
Before I spend too much time, is this a new file or a renamed file? If renamed are there additional edits to the content? It looks like new file and all new content. If this is a rename, git can track file renames and changes after the rename if the events are committed separately. And these differences can be seen during review to make discerning old/new easier.
It's a rename via "git mv connectionevent triggertrace" and additional edits to combine the "connection" and "connectionevent" packages into the "triggertrace" package.
SaiedKazemi
commented
2 years ago caller.go, line 39 at r1 (raw file):
I'm confused. Why move flags to individual packages?
I noticed that other traceroute-caller packages like ipcache (and now removed connectionpoller) define their own package flags. Also, outside TRC, packages like ipservice and eventsocket do the same. So for consistency I moved these to the tracer package. However, we should actually remove package-level flags because our internal guideline says that "General-purpose packages should be configured using function arguments or global variables, not by punching through to the command-line interface."
SaiedKazemi
commented
2 years ago caller.go, line 39 at r1 (raw file):
We discussed in person, and largely agreed that flags likely belong in main for traceroute-caller packages.
I have moved all flags to the main package.
SaiedKazemi
commented
2 years ago caller.go, line 33 at r1 (raw file):
We discussed in person. We see things from different perspectives. But, this difference may not matter much.
Personal perspectives aside, the main blocker for using rtx.Must() is that it calls os.Exit(1), preventing us to test the main() function with invalid arguments.
SaiedKazemi
commented
2 years ago hopannotation/hopannotation.go, line 106 at r1 (raw file):
We discussed in person. We see things from different perspectives. But, this difference may not matter much.
I have changed all factory functions to require explicit values. They do not assume any default values anymore.
SaiedKazemi
commented
2 years ago triggertrace/triggertrace.go, line 1 at r1 (raw file):
This comes up a lot for myself and others too. I'll see if I can identify the preferable sequence to recommend in the future.
That would be nice. I'll be happy to join you and do this together.
SaiedKazemi
commented
2 years ago triggertrace/triggertrace.go, line 82 at r1 (raw file):
It looks safe to me (only references to LocalIPs, no writes). If you determine differently, please let me know - I'd like to see what I didn't see.
I also think it's safe, but I am keeping it as is in this PR to limit the changes to refactoring the code only (i.e., no application logic changes).
SaiedKazemi
commented
2 years ago triggertrace/triggertrace.go, line 85 at r1 (raw file):
Out of curiosity why dereference the sockID?
Because SockID is a pointer and findDestination() takes inetdiag.SockID. But I changed findDestination() to take a pointer, so there's no need to dereference.
SaiedKazemi
commented
2 years ago triggertrace/triggertrace.go, line 92 at r1 (raw file):
Should this return before falling through? Otherwise the key is the empty string, right?
The original code didn't even examine uuid. I was curious if uuid can ever be the empty string and added this if block but I did not want to change the application logic by returning.
SaiedKazemi
commented
2 years ago Thank you for your time and feedback. I hope the refactored code is a lot easier to read, reason about, and maintain.
SaiedKazemi
commented
2 years ago caller.go, line 33 at r1 (raw file):
How did you work around the call to `os.Exit` with `log.Fatal`? :)
By introducing the variable logFatalf and re-assigning it in caller_test.go. You can see the details in TestMainWithBadArgs().
SaiedKazemi
commented
2 years ago triggertrace/triggertrace.go, line 85 at r1 (raw file):
Oh, this is another instance of "no logic changes" in the refactor? Sorry -- it's also just hard to tell what's changed so easier for me to take things as "new" even if they weren't.
Yeah, it's confusing even for me sometimes. In this case, the change was really minor, so effectively there's no application logic change.
SaiedKazemi
commented
2 years ago I will merge this set of changes. The next PR will be for adding the package unit tests.
This commit refactors traceroute-caller code to make the code easier to read and maintain, and also to make it more conformant with Go style guidelines.
There is no change in the application logic.
This is a work in progress and will be followed by other commits. Specifically, package testing code that is missing from this commit will be added soon.
The main purpose of this commit is to facilitate the review process.
This change is