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

Various code cleanups #17

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

Various code cleanups #17

wants to merge 4 commits into from

Conversation

Keitio
Copy link

@Keitio Keitio commented Sep 23, 2022

  • Added context.Context to most top-level functions
  • Renamed errors to follow standard Go pattern
  • Refactored interface{} to any
  • Renamed method receivers to more meaningful names (self or this are bad practice in Go)
  • Cleaned some select and switch uses
  • Some small performance improvements (less channels created)
  • Other various cleanups
    If this is too much change for one PR, I can split it to smaller ones

@tobiemh
Copy link
Member

tobiemh commented Sep 25, 2022

Hey @Keitio the current interface{} values were originally any, before they got changed back to interface{} here: ae66148

Not too sure why, but there was a reason. I haven't used Go in a while now, so not up-to-date with the latest practices around any. Will this continue to work for older version of go?

@Keitio
Copy link
Author

Keitio commented Sep 25, 2022

Basically, any is declared in go 1.18+ as an alias to interface{}, so it will not work for older versions
But the go.mod says this package is made for 1.18, so maybe we want to change that ?
I personally would prefer to stay with 1.18, because we get nice any, and more importantly generics and fuzzing in the standard library, which would be nice for tests

Maybe we could maintain some sort of very basic connector compatible with 1.13+, and this one as the official connector, but only 1.18+ ?

Ultimately it's your call whether we want compatibility or nice stuff, but the go.mod should reflect that anyway

@Keitio
Copy link
Author

Keitio commented Sep 25, 2022

I reverted the change from interface{} to any, and changed the go.mod to be 1.13
@tobiemh would that be okay with you ? We can always bump the go.mod later when needed, but the nice APIs enabled by generics will not work
But we get way better compatibility, as go <1.13 is very seldom used today (if at all)

@tobiemh
Copy link
Member

tobiemh commented Sep 27, 2022

@Keitio I have been out of the Go world for a bit now (since 1.16), so not up-to-date with usage.

What is the usage of Golang pre 1.18 currently?

@Keitio
Copy link
Author

Keitio commented Sep 28, 2022

I can only speak from experience and inference, but most use i saw was from at most 2 versions prior to latest (that would mean 1.17 currently)
Most support from the Go team is also limited to current-2, so i would not find it absurd to limit to 1.18+
But i worked mostly in startup-like environments (i think that accounts for a lot of Go use) so my experience may be skewed

@intabulas
Copy link
Contributor

I concur on the current-2, especially in enterprise-y environments where people are discouraged from running on the bleeding edge.

@tobiemh
Copy link
Member

tobiemh commented Oct 18, 2022

Hi @Keitio could you rebase the current main branch and pull in the latest changes?

@Keitio
Copy link
Author

Keitio commented Oct 18, 2022

Done, need a maintainer to give the green light to workflows

db.go Outdated Show resolved Hide resolved
types.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
db_test.go Outdated Show resolved Hide resolved
@tobiemh
Copy link
Member

tobiemh commented Oct 19, 2022

Hey @Keitio just a few minor changes 😀👍👏 !

@Keitio
Copy link
Author

Keitio commented Oct 19, 2022

No problem, I don't think I'll get to it today but it will be done tomorrow

@Keitio
Copy link
Author

Keitio commented Nov 2, 2022

Well, a bit late (sorry for that) but i rebased on main and applied your changes

@phughk phughk self-assigned this Mar 7, 2023
Copy link
Contributor

@phughk phughk left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! It might have merge conflicts once we land another change, but otherwise approved. Thank you!

@Keitio
Copy link
Author

Keitio commented Mar 8, 2023

Merge should be all good, tests pass locally

@phughk
Copy link
Contributor

phughk commented Mar 9, 2023

Hey @Keitio , I just fixed the lining fault in our pipeline, so I am surprised you are getting that as I thought the linter would rebase. Guess it doesn't. Would you be able to bring the latest changes in please? To run the linter against it properly.

@phughk phughk added the enhancement New feature or request label Mar 9, 2023
Keitio and others added 3 commits March 9, 2023 21:49
Add testify to dependencies
Rewrite tests to use testify
Ignore test that has a permission issue
@Keitio
Copy link
Author

Keitio commented Mar 9, 2023

i hope this is good, merge was a bit chaotic on my end

@phughk phughk added the stale label Apr 30, 2023
@phughk
Copy link
Contributor

phughk commented Apr 30, 2023

Hey @Keitio ! Nearly ready to merge! All you need to do is resolve the conflicts, and we can add this :) Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants