Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
String slice issue v1 #981
String slice issue v1 #981
Changes from 4 commits
c34587c
9a9e396
a5a927c
9b12ddb
dfb55ca
94e50b3
809a71d
0f5ca21
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We shouldn't be using named returns unless the function signature is ambiguous, or we absolutely have to
There are caveats with the scope of a named variable that aren't always intuitive, and naked returns pretty much always obfuscate what code should be doing. It also encourages us to mutate an existing variable rather than returning directly.
I'd probably write the function like this:
That said, there's an issue here, which I'm not sure if is intentional or not. This function removes all items up to and including the first occurrence of
val
, but the name implies that we would just be removingval
. I don't think it manifests because of how this function is called, but we should either fix the implementation or rename the function to avoid future misuse of this helper.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the function to behave like it's named
See it in action: https://play.golang.org/p/ZjJxp38tKNQ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first time I every see an expression like this in golang. 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woooooooooah, love it!