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

unit: use ContainsAny instead of IndexAny #263

Merged
merged 2 commits into from
Oct 31, 2018

Conversation

muesli
Copy link
Contributor

@muesli muesli commented Mar 20, 2018

Caution: If I'm not mistaken ContainsAny wasn't available before Go 1.5. I'm not sure which minimum Go version go-systemd is aiming for, but if it's still <1.5 then you might not want to merge this PR.

That being said, I think it reads nicer.

Edit: I was mistaken, bytes.ContainsAny was introduced with the 1.7 release.

@lucab
Copy link
Contributor

lucab commented Mar 20, 2018

I have a minimum Go version bump (to >=1.10) on my roadmap, but I'm waiting for at least Go 1.11 to be released before doing that. I think I'm going to merge this PR at that later point. Bumping will also require some Travis adjustment.

For the moment, I'll keep this open and wait. In general it looks good, thanks a lot!

@lucab lucab self-requested a review March 20, 2018 11:03
@lucab lucab changed the title Use ContainsAny instead of IndexAny unit: use ContainsAny instead of IndexAny Mar 20, 2018
@lucab lucab added this to the v18 milestone May 7, 2018
@lucab
Copy link
Contributor

lucab commented May 23, 2018

@muesli I just merged #278 which should make testing against specific go versions easier. Feel free to rebase this PR on master and adjust the minimum go version as needed.

@muesli
Copy link
Contributor Author

muesli commented May 23, 2018

@lucab Will do!

@muesli muesli force-pushed the unit-bytes-contains branch from 2e2dbf4 to 8b7ed52 Compare May 23, 2018 15:45
@muesli
Copy link
Contributor Author

muesli commented May 23, 2018

Pushed. This would bump the minimum required Go version to 1.7.

I'm not sure it's worth bumping the requirement for this change alone, though, to be honest. That being said, 1.7 also came with context, which would probably open up the door for some other nice changes in go-systemd.

Happy to keep this alive until we eventually decide to bump the requirement.

@lucab
Copy link
Contributor

lucab commented May 23, 2018

Ack, I'll keep this around open. We can queue it anytime in the future when we will need to bump the toolchain.

@lucab lucab modified the milestones: v18, v19 Oct 4, 2018
@lucab
Copy link
Contributor

lucab commented Oct 30, 2018

@muesli mind rebasing on current master? Minimum toolchain just bumped to 1.7 due to go-dbus anyway.

@muesli
Copy link
Contributor Author

muesli commented Oct 30, 2018

Will do in a minute!

@muesli muesli force-pushed the unit-bytes-contains branch 2 times, most recently from 61c1c7b to cdcfe50 Compare October 30, 2018 21:58
@muesli
Copy link
Contributor Author

muesli commented Oct 30, 2018

@lucab Good to go from my POV.

@muesli
Copy link
Contributor Author

muesli commented Oct 30, 2018

The one failed Travis job looks like a fluke / unlikely name clash to me?

Edit: I'll force push to my branch to test that theory...

@muesli muesli force-pushed the unit-bytes-contains branch from cdcfe50 to 7827196 Compare October 30, 2018 22:17
@lucab
Copy link
Contributor

lucab commented Oct 31, 2018

Yes, it's a known flake: #290

I re-triggered and it's green now, merging. Thanks a lot (with some delay 😄)!

@lucab lucab modified the milestones: v19, v18 Oct 31, 2018
@lucab lucab merged commit 9002847 into coreos:master Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants