nat: assorted optimisations and test-impovements#121
nat: assorted optimisations and test-impovements#121thaJeztah merged 5 commits intodocker:masterfrom
Conversation
thaJeztah
changed the title
thaJeztah
force-pushed
the
nat_small_optim
branch
from
9bdb680 to
de93d92
Compare
robmry
left a comment
robmry
left a comment
There was a problem hiding this comment.
Some non-blocking suggestions ...
nat/nat.go
Outdated
| // SplitProtoPort does not validate or normalize the returned values. | ||
| func SplitProtoPort(rawPort string) (string, string) { |
There was a problem hiding this comment.
Splitting "port/proto" and returning proto, port (instead of port, proto) is surprising. Could name the return vals? ...
| // SplitProtoPort does not validate or normalize the returned values. | |
| func SplitProtoPort(rawPort string) (string, string) { | |
| // SplitProtoPort does not validate or normalize the returned values. | |
| func SplitProtoPort(rawPort string) (proto, port string) { |
There was a problem hiding this comment.
Ah, yes, can do; I was actually considering that, as I also was a bit surprised that this one flipped the order of the fields (was even considering to mark it deprecated as it's a bit of a silly function, but the replacement (use strings.Cut) would also require users to fill in the default (tcp), so there's still "some" value.
| } | ||
|
|
||
| p, err = ParsePort("1asdf") | ||
| for _, tc := range tests { |
There was a problem hiding this comment.
1asdf (number followed by junk) seems like a good test to keep?
There was a problem hiding this comment.
Oh! Yes, possibly. I completely read that one as lasdf (lowercase L 😂); will add it back, good catch!
nat/nat_test.go
Outdated
| // FIXME currently this is a valid port. I don't think it should be. | ||
| // I'm leaving this test commented out until we make a decision. | ||
| // - erikh | ||
| // { | ||
| // doc: "octal value", | ||
| // port: "0123", | ||
| // expPort: 0, | ||
| // expErr: `some error message`, | ||
| // }, |
There was a problem hiding this comment.
In the absence of a decision - it's probably worth un-commenting this to record current behaviour, in case it changes and someone's ended up relying on it? (Then a change would have to be deliberate.)
- validate protocol before doing additional parsing - use a switch instead of looping over supported protocols - lowercase the proto once - remove some uses of fmt.Sprintf in favor of string-concatenating Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use strings.Cut to improve performance and simplify the code, and update the GoDoc to better outline expectations. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Unwrap the error to don't include "strconv.ParseUint" in the error message. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
nat_small_optim
branch
from
de93d92 to
727d32d
Compare
|
@robmry updated; PTAL 🤗 |
2 participants
nat: slight optimization in ParsePortSpec
nat: TestSplitProtoPort: use table-test
nat: SplitProtoPort: optimize and improve GoDoc
Use strings.Cut to improve performance and simplify the code,
and update the GoDoc to better outline expectations.
nat: TestParsePort: use table-test
nat: ParsePort: improve error
Unwrap the error to don't include "strconv.ParseUint" in the
error message.
- A picture of a cute animal (not mandatory but encouraged)