lang: funcs: Validates mac address strings #649

S-ign · 2021-03-09T19:23:42Z

Tips:

please read the style guide before submitting your patch:
docs/style-guide.md
commit message titles must be in the form:

topic: Capitalized message with no trailing period

or:

topic, topic2: Capitalized message with no trailing period

golang code must be formatted according to the standard, please run:

make gofmt		# formats the entire project correctly

or format a single golang file correctly:

gofmt -w yourcode.go

please rebase your patch against current git master:

git checkout master
git pull origin master
git checkout your-feature
git rebase master
git push your-remote your-feature
hub pull-request	# or submit with the github web ui

after a patch review, please ping @purpleidea so we know to re-review:

# make changes based on reviews...
git add -p		# add new changes
git commit --amend	# combine with existing commit
git push your-remote your-feature -f
# now ping @purpleidea in the github PR since it doesn't notify us automatically

Thanks for contributing to mgmt and welcome to the team!

S-ign · 2021-03-09T19:43:13Z

tests passed

purpleidea

Hey @S-ign ! Thanks for the patch.

Structurally everything looks good! I left some comments. Make sure to review each one. If there's something you don't understand or agree with, leave a comment instead of updating things in your new patch.

For small patches, I generally recommend fixing things and force-pushing a new version.
For enormous patch sets, it's usually better to add a new commit with the fixes, and then rebase squash right at the end. (We have the privilege of hiding all of our mistakes before we commit them to git master forever!)

As for the commit message, try re-wording it and adding a small sentence or two describing anything unusual or special about this patch. Have a go.

One last tip: Usually the flow most people are used to looks like: 1) hack, hack, hack; 2) commit 3) push

I recommend adding a step 2.5 which involves reading your entire PR as if you were doing a review. This is helpful to both get better at reading code, and to make sure you have everything as you want it.

Thanks again!

lang/funcs/core/net/is_macaddress_func.go

purpleidea · 2021-03-10T03:24:50Z

lang/funcs/core/net/is_macaddress_func_test.go

+ output, err := IsMacAddress([]types.Value{
+ &types.StrValue{V: test.mac},
+ })
+ expectedStr := &types.BoolValue{V: test.expected}


we're expecting a str? be careful of copy+pasta errors.

ok, so it has to be string, thought I could return other types too.

I didn't understand your reply, can you rephrase?

I see now, I initially changed everything else to return bool but this variable was named expectedStr so it didn't make sense.
I'm going to change my function so that it returns string "true" or "false" instead.

purpleidea · 2021-03-10T03:25:38Z

lang/funcs/core/net/is_macaddress_func_test.go

+ expectedStr := &types.BoolValue{V: test.expected}
+
+ if test.err != nil && err.Error() != test.err.Error() {
+ t.Errorf("mac %s, expected error: %q, got %q", test.mac, test.err, err)


not sure actually, I've copied this test.

Never copy code you don't 100% understand ;) Dig into it and make sure it's what you want.

looks like i didn't need a lot of those if statements because I'm not really checking for err since its always nil.
should the function even return err if its always nil?

purpleidea · 2021-03-10T03:25:59Z

lang/funcs/core/net/is_macaddress_func_test.go

+ t.Errorf("mac: %s, expected error: %v, but got nil", test.mac, test.err)
+ return
+ } else if test.err == nil && err != nil {
+ t.Errorf("mac %s, did not expect error but got: %#v", test.mac, err)


purpleidea · 2021-03-10T03:26:31Z

lang/funcs/core/net/is_macaddress_func_test.go

+ {"invalid mac invalid delimiter position", "0a--1b2-c3d-4f-56", false, nil},
+ }
+
+ for _, test := range macAddressTests {


add test := test right here...

I don't understand, I'm replacing
for _, test := range macAddressTests { t.Run(test.name, func(t *testing.T) { output, _ := IsMacAddress([]types.Value{ &types.StrValue{V: test.mac}, })

for:

test := test

wouldn't that get rid of the loop that goes through all the tests? I'm confused about what this does.
I tried it, but it just breaks the test.

purpleidea

Okay. I hope you've been learning from the review process. There's one big change we didn't make yet. I forgot if I delayed telling you on purpose, or if I forgot.

We'd definitely like to re-use existing code from the stdlib wherever possible. First, because that saves time. Second, because that code is probably tested more often. Third, because it is probably more optimized.

Unfortunately for your patch (but not for your learning experience) there's already a mac address parser in the stdlib. It may not be exactly what we want, but we probably want to use it somehow.

So have a look, find it, update this code and anything else, and please push a new version.

Use your newly git skills to combine these commits with the new code you're about to write into a single commit.

Thanks!

purpleidea · 2021-03-17T04:57:38Z

lang/funcs/core/net/is_macaddress_func.go

+
+// IsMacAddress takes a single string mac address as input, checks that it
+// contains one of the accepted delimiter [":","-","."], checks that all non-
+// numarical chars are between a - f, and that the delimiter is separating 2


purpleidea · 2021-03-17T04:58:15Z

lang/funcs/core/net/is_macaddress_func.go

+
+// IsMacAddress takes a single string mac address as input, checks that it
+// contains one of the accepted delimiter [":","-","."], checks that all non-
+// numarical chars are between a - f, and that the delimiter is separating 2


nit: 2 -> two.
(numbers less than 10 are usually written as words)

purpleidea · 2021-03-17T05:00:23Z

lang/funcs/core/net/is_macaddress_func.go

+ })
+}
+
+// IsMacAddress takes a single string mac address as input, checks that it


So this comment is describing the code. And we don't really need to do that for short snippets of code, because well, the code does that. And if we ever changed one of these two, then things would be out of sync.

More importantly, what we want to do, is tell the user and important things. It might be important to tell them which type of macaddresses we validate, and that sort of thing. Talking about the separators is good though.

purpleidea · 2021-03-17T05:02:00Z

lang/funcs/core/net/is_macaddress_func_test.go

+ expectedBool := &types.BoolValue{V: test.expected}
+
+ if err1 := output.Cmp(expectedBool); err1 != nil {
+ t.Errorf("mac: %s, expected: %s, got: %s", test.mac, expectedBool, output)


What is %s for? Are we using the right format string everywhere?

%s is for the uninterpreted bytes of the string or slice, maybe i should us %v for the string default value.

purpleidea · 2021-03-17T05:03:14Z

lang/funcs/core/net/is_macaddress_func_test.go

+ for _, test := range macAddressTests {
+ t.Run(test.name, func(t *testing.T) {
+ test := test
+ output, _ := IsMacAddress([]types.Value{


never ignore errors without a comment saying why or a really good reason.

alright, I'll leave a comment saying that err will always return nil.

purpleidea requested changes Mar 10, 2021

View reviewed changes

purpleidea requested changes Mar 17, 2021

View reviewed changes

lang: funcs: Validates mac address strings

65e5271

S-ign force-pushed the feat/fedora branch from 0ab9a41 to 65e5271 Compare March 22, 2021 16:31

purpleidea force-pushed the master branch from 73f82c8 to 4d18741 Compare May 4, 2021 09:30

purpleidea force-pushed the master branch 4 times, most recently from 2319f4f to 04f5ba6 Compare September 12, 2022 01:44

purpleidea force-pushed the master branch 4 times, most recently from fe2313c to 271a94e Compare December 4, 2023 02:35

purpleidea force-pushed the master branch from b08136d to 78e59a9 Compare January 22, 2024 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lang: funcs: Validates mac address strings #649

lang: funcs: Validates mac address strings #649

S-ign commented Mar 9, 2021

S-ign commented Mar 9, 2021

purpleidea left a comment

purpleidea Mar 10, 2021

S-ign Mar 10, 2021

purpleidea Mar 10, 2021

S-ign Mar 10, 2021

purpleidea Mar 10, 2021

S-ign Mar 10, 2021

purpleidea Mar 10, 2021

S-ign Mar 10, 2021

purpleidea Mar 10, 2021

purpleidea Mar 10, 2021

S-ign Mar 10, 2021

purpleidea left a comment

purpleidea Mar 17, 2021

purpleidea Mar 17, 2021

purpleidea Mar 17, 2021

purpleidea Mar 17, 2021

S-ign Mar 22, 2021

purpleidea Mar 17, 2021

S-ign Mar 22, 2021

lang: funcs: Validates mac address strings #649

Are you sure you want to change the base?

lang: funcs: Validates mac address strings #649

Conversation

S-ign commented Mar 9, 2021

Tips:

Thanks for contributing to mgmt and welcome to the team!

S-ign commented Mar 9, 2021

purpleidea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purpleidea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment