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

Upgrade to go 119 #522

Merged
merged 12 commits into from
Oct 2, 2023
Merged

Upgrade to go 119 #522

merged 12 commits into from
Oct 2, 2023

Conversation

smallhive
Copy link
Contributor

Close #492

closes #492

Signed-off-by: Evgenii Baidakov <[email protected]>
Signed-off-by: Evgenii Baidakov <[email protected]>
@smallhive smallhive marked this pull request as ready for review September 21, 2023 07:43
pool/pool.go Outdated Show resolved Hide resolved
@carpawell
Copy link
Member

Can you, please, do the same as we did in the node repo? Is allows us to be sure this PR broke nothing in the other places.

@@ -117,11 +117,14 @@ type clientStatusMonitor struct {
}

func newClientStatusMonitor(addr string, errorThreshold uint32) clientStatusMonitor {
return clientStatusMonitor{
m := clientStatusMonitor{
Copy link
Member

@carpawell carpawell Sep 21, 2023

Choose a reason for hiding this comment

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

well, i got through this in the node repo but that is the second time i see how adapting to sync makes it harder to read and adds more lines. so maybe add some helper atomic constructor func to our repos?

Copy link
Member

Choose a reason for hiding this comment

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

Please, no. Frankly, I've expected the stdlib sync to be somewhat more friendly, but it's not that bad either. The best thing here is that it's a standard one.

Copy link
Member

Choose a reason for hiding this comment

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

love keeping things as standard as they are so wont argue about that

I've expected the stdlib sync to be somewhat more friendly

but +1 here

pool/pool.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@carpawell
Copy link
Member

Also, the first two commits are the same for me. But up to you.

Original message is - copylocks: return copies lock value: github.com/nspcc-dev/neofs-sdk-go/pool.clientStatusMonitor contains sync.RWMutex.

Signed-off-by: Evgenii Baidakov <[email protected]>
Version v0.6/v1 is out of support. V2 brings generics and makes out code a little bit easier.

Signed-off-by: Evgenii Baidakov <[email protected]>
Signed-off-by: Evgenii Baidakov <[email protected]>
Sometimes touching one line of code breaks the other and we're 100% clean now.

Signed-off-by: Evgenii Baidakov <[email protected]>
@smallhive
Copy link
Contributor Author

smallhive commented Sep 22, 2023

Can you, please, do the same as we did in the node repo? Is allows us to be sure this PR broke nothing in the other places.

Works perfectly https://github.com/nspcc-dev/neofs-sdk-go/actions/runs/6269928793/job/17027095101?pr=522 😅

Do we suppress these errors? Fixing them may significantly affect the SDK API

@roman-khimov
Copy link
Member

As for deprecation warnings, you can suppress them if there are no better ways (although it's somewhat strange to have something deprecated and not have a replacement).

@smallhive
Copy link
Contributor Author

As for deprecation warnings, you can suppress them if there are no better ways (although it's somewhat strange to have something deprecated and not have a replacement).

Yes, maybe we should pay more attention to #25.

But what about:

  • Error: Error return value of sig.ReadFromV2 is not checked (errcheck) here
  • Error: Error return value of ver.ReadFromV2 is not checked (errcheck) here

panic? Changing the function API to return an error may significantly influence some code.

@roman-khimov
Copy link
Member

With the current API you can return nil.

@smallhive
Copy link
Contributor Author

Updated

@carpawell
Copy link
Member

although it's somewhat strange to have something deprecated and not have a replacement

That is because of nspcc-dev/neofs-api#205. No need to store SG's expiration as a payload's part if it is always an object that has its own expiration. So maybe deprecate SDK's getters too? Or even remove them. @smallhive, @roman-khimov, @cthulhu-rider

storagegroup/storagegroup.go Show resolved Hide resolved
object/object.go Show resolved Hide resolved
object/object.go Outdated Show resolved Hide resolved
@roman-khimov roman-khimov merged commit e2717d2 into master Oct 2, 2023
5 checks passed
@roman-khimov roman-khimov deleted the 492-upgrade-to-go-119 branch October 2, 2023 07:22
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.

Upgrade to Go 1.19+
3 participants