Optimize computeRoutePrefix #584
Conversation
Previously, computeRoutePrefix was adding a '/' to the start of the prefix and then removing it from the end, regardless of whether it was there or not. This resulted in unnecessary string operations. The updated function only adds or removes the '/' when necessary, improving efficiency. Additionally, the function now uses strings.HasPrefix and strings.HasSuffix for clearer intent and readability. Signed-off-by: donuts-are-good <[email protected]>
|
Thank you. I'll have a look ASAP. Could you fix the gofmt issue flagged by the CI test in the meantime? |
|
Good morning @beorn7, and thank you for the reply. I will do that when I return to my desk this afternoon after family time :) |
|
Bit embarrassed to ask, but what is it that is needed here?
|
|
Maybe you are using an old Go version? Line 10 is a spurious blank line. |
|
I'm using 1.21.1, I'll see if removing that line helps. Thanks for the tip! |
This change is to remove an extra blank line above shutdownServerOnQuit() that could possibly be the source of a CI test error. Signed-off-by: donuts-are-good <[email protected]>
|
Yahtzee! |
|
Is there anything else I should do, or just sit tight and wait? |
There was a problem hiding this comment.
Thank you. Looks good in general, but L263 isn't needed, and I have some Promethean nit picking about punctuation. See comments.
Apologies for the review delay. I generally have to fight a lengthy review backlog, and on top of that, I have been at SREcon all week long.
Sounds good, I'll get this taken care of early this week. Hope it was a good trip, thanks for the feedback :) |
It was pointed out to me that these comments were lacking final punctuation. This commit adds punctuation. Signed-off-by: donuts-are-good <[email protected]>
It was pointed out that strings.TrimSuffix already contained the functionality added here, removing the conditional that added HasSuffix Signed-off-by: donuts-are-good <[email protected]>
|
What an awesome gift to see when I log in to Github today. Thank you for your time and guidance on this PR @beorn7 |
Previously, computeRoutePrefix was adding a '/' to the start of the prefix and then removing it from the end, regardless of whether it was there or not. This resulted in unnecessary string operations. The updated function only adds or removes the '/' when necessary, improving efficiency. Additionally, the function now uses strings.HasPrefix and strings.HasSuffix for clearer intent and readability.