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

Incorporate minor updates from the standard library #26

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

djdv
Copy link

@djdv djdv commented Jul 5, 2022

Changes proposed in this pull request

Migrating this over from: spf13#353
Figured you might be interested in it.


While reading the source for this pkg, I noticed the usage of bytes.Buffer in places where Go 1.10's strings.Builder could be used instead.
(It's intended for this and should be marginally more performant by avoiding some copies internally)

While migrating over, I found a handful of other small linting changes that seem acceptable.
All that is included here, but anything can be dropped if it's not desired.

I don't expect this to change any of observable behavior and the tests pass, but who knows.


Extra text not in the original PR:
I might have to rebase this if you don't want to pull in the missing commit from their master as well.

Also 2433e5b is likely better than no up-front allocation, but still isn't the exact size.
So I'm only guessing that this does less reallocs than it did before, but probably has to do at least 1.
We can either try to improve this logic or just drop it.

The commits are split up for review but can get squashed or something too.

Checklist

  • [❌] Tests have been added and/or updated
    N/A
  • [❌] make test has been run
    go test .\... was used instead and passes.
    Cobra is thus not tested with this.
  • [⭕] make lint has been run
    There are some gofmt issues, but they're not introduced by me.

@djdv djdv changed the title Std updates fork Incorporate minor updates from the standard library Jul 5, 2022
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.

2 participants