Skip to content
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

fixed declaration dependencies and other lint issues in code base #1743

Merged
merged 1 commit into from Aug 10, 2020

Conversation

varun06
Copy link
Collaborator

@varun06 varun06 commented Jul 5, 2020

Another lint/vet fix that's backward compatible.

+--------+---------+---------------------------+-------+------+------+------+
| STATUS | ELAPSED |          PACKAGE          | COVER | PASS | FAIL | SKIP |
+--------+---------+---------------------------+-------+------+------+------+
| PASS   | 76.81s  | github.com/Shopify/sarama | 0.0%  |  446 |    0 |    1 |
+--------+---------+---------------------------+-------+------+------+------+

@varun06 varun06 requested a review from bai as a code owner Jul 5, 2020
@varun06 varun06 requested review from dnwe and d1egoaz Jul 5, 2020
replicas := []int32{}
offlineReplicas := []int32{}
var replicas []int32
var offlineReplicas []int32
Copy link
Contributor

@d1egoaz d1egoaz Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this one:
https://play.golang.org/p/vdAdcwi2i09

func main() {
	a := []int32{}
	var b []int32
	
	fmt.Printf("a: %#v\n", a)
	fmt.Printf("b: %#v\n", b)
}

result:
a: []int32{}
b: []int32(nil)

Copy link
Collaborator Author

@varun06 varun06 Jul 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, yeah, that does changes semantics. but as far as I know, An empty slice can be represented by nil or an empty slice literal. The first approach is preferred as it does not lead to memory allocation. also tests are passing with changing to nil slice declaration. Let me know if that sounds good?

Copy link
Collaborator Author

@varun06 varun06 Jul 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@d1egoaz d1egoaz Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also tests are passing with changing to nil slice declaration

not sure if we are in a position to trust in the tests 😅

have you tried this branch in a running app?

Copy link
Contributor

@d1egoaz d1egoaz Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked some folks, and they mentioned that before the variable was initialized to an empty array, with your change the array is now nil.

However, append will initialize the array internally

// The append built-in function appends elements to the end of a slice. If
// it has sufficient capacity, the destination is resliced to accommodate the
// new elements. If it does not, a new underlying array will be allocated.

https://play.golang.org/p/4EHaAlO5eIG

so, 🤞 it looks should work

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go makes 'zero-values' do the expected thing. For slices, append, len, cap, range all behave well.

Copy link
Contributor

@d1egoaz d1egoaz left a comment

it'd be great if you can try this branch first on an existing app

@varun06 varun06 merged commit 25aedae into Shopify:master Aug 10, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants