purpleidea / mgmt

Next generation distributed, event-driven, parallel config management!
https://purpleidea.com/tags/mgmtconfig/
GNU General Public License v3.0
3.67k stars 315 forks source link

lang: ast: Remove redundant nil check #731

Closed Juneezee closed 9 months ago

Juneezee commented 10 months ago

From the Go specification ^1:

"1. ... For a nil slice, the number of iterations is 0."

Therefore, an additional nil check for around the loop is unnecessary.

/cc @purpleidea for review

Juneezee commented 10 months ago

@purpleidea I apologize for the nil pointer panic introduced in the first commit. I have pushed a fix for the panic. Please approve the CI runs again :bowing_man: .

CI runs in my fork are now green https://github.com/Juneezee/mgmt/actions/runs/7677576810

purpleidea commented 9 months ago

@Juneezee Thanks for the patch-- just a quick question or two if you don't mind:

1) What tool did you use to find these changes? 2) Are you interested in making some bigger mgmt patches? 3) Thanks! image

Juneezee commented 9 months ago

just a quick question or two if you don't mind:

@purpleidea Yup, I don't mind :smile:

  1. What tool did you use to find these changes?

GitHub search. I was searching for "abstract syntax tree" to find some code for learning purposes. This repository is one of the top results on the first page.

This was the search query that I used: "abstract syntax tree" lang:Go -repo:golang/go

image

  1. Are you interested in making some bigger mgmt patches?

Yes! Absolutely!

  1. Thanks! image

1997, my birth year Oops, largest even prime number :smile:, 2

purpleidea commented 9 months ago

What tool did you use to find these changes?

Thanks for the info-- sorry what I mean was, what linting tool did you use to find the specific code changes to make, or did you read through the whole file and decide on them manually?

Are you interested in making some bigger mgmt patches? Yes! Absolutely!

Okay great! Want to give me a quick summary of your strengths and weaknesses in golang, if you're comfortable with golang concurrency, and what you'd like to work on. Eg: parser stuff, a resource, a function, concurrency bugs etc...

I can help you choose and mentor you on your patches. Cheers!

Juneezee commented 9 months ago

Thanks for the info-- sorry what I mean was, what linting tool did you use to find the specific code changes to make, or did you read through the whole file and decide on them manually?

I did skim through the whole file. I use GoLand and I noticed the slice declaration warning, which is just 1 line above my change.

image

Okay great! Want to give me a quick summary of your strengths and weaknesses in golang, if you're comfortable with golang concurrency, and what you'd like to work on. Eg: parser stuff, a resource, a function, concurrency bugs etc...

I can help you choose and mentor you on your patches. Cheers!

I would say I'm pretty confident in normal day-to-day Go programming, and I'm comfortable with Go concurrency too.

However, I lack experience with project architecture and engineering practices (stuff like that). If there are issues or feature requests that you think are beginner-friendly, feel free to assign them to me and I am happy to work on it :smile:

Thank you very much!

purpleidea commented 9 months ago

slice declaration warning

Interesting, I didn't consider this a bad practice. I'll look into it more.

feel free to assign them to me and I am happy to work on it 😄

Okay, since you mentioned AST's and compilers, I wrote up an issue over here... https://github.com/purpleidea/mgmt/issues/732 If you're interested in that one, LMK and comment over there that you're working on it. If you want something different, please LMK.

purpleidea commented 9 months ago

LGTM, let's see what CI says... Thanks!

purpleidea commented 9 months ago

Merged, thank you!

What's next?

Juneezee commented 9 months ago

Merged, thank you!

What's next?

I'll be looking into https://github.com/purpleidea/mgmt/issues/732, and I hope I can solve it