search

nspcc-dev / neofs-sdk-go

Go implementation of NeoFS SDK
Apache License 2.0
6 stars 14 forks source link

Place the compiled NeoFS protocol files in the repository #591

Closed cthulhu-rider closed 4 months ago

cthulhu-rider commented 4 months ago

they are not used yet, but will be in the next PRs

cthulhu-rider commented 4 months ago

Have you considered moving "SDK" things to a separate folder?

no i havent. I won’t say anything against it right off the bat, imo u should create an issue to reorganize the tree. Go source code may be placed into src dir, or pkg+internal

I have tested it and a few generated files were overwritten, have you used the latest API?

true, i missed some latest commits. Now everything should be fine

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 0% with 4889 lines in your changes missing coverage. Please review.

Project coverage is 39.47%. Comparing base (11ca22b) to head (4a0be23). Report is 5 commits behind head on master.

Files Patch % Lines
api/session/types.pb.go 0.00% 553 Missing :warning:
api/netmap/types.pb.go 0.00% 493 Missing :warning:
api/netmap/service.pb.go 0.00% 465 Missing :warning:
api/acl/types.pb.go 0.00% 390 Missing :warning:
api/refs/types.pb.go 0.00% 378 Missing :warning:
api/object/types.pb.go 0.00% 371 Missing :warning:
api/reputation/service.pb.go 0.00% 329 Missing :warning:
api/reputation/types.pb.go 0.00% 213 Missing :warning:
api/status/types.pb.go 0.00% 191 Missing :warning:
api/session/service.pb.go 0.00% 185 Missing :warning:
... and 15 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #591 +/- ## =========================================== - Coverage 68.18% 39.47% -28.71% =========================================== Files 122 150 +28 Lines 10035 17594 +7559 =========================================== + Hits 6842 6946 +104 - Misses 2814 10269 +7455 Partials 379 379 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

roman-khimov commented 4 months ago
  1. Do not move anything we have currently public, this SDK is used a lot.
  2. proto or rawproto may be a bit less ambiguous than api.
  3. internal/api can be considered as well, although these definitions can be useful in some ways.
cthulhu-rider commented 4 months ago

Do not move anything we have currently public, this SDK is used a lot.

overall agree, #594 is dedicated for the discussion. There are backward compatible ways to do such things

proto or rawproto may be a bit less ambiguous than api.

i dont mind, suggest moving? I thought api matches well with the names of the corresponding repository and protocol

internal/api can be considered as well, although these definitions can be useful in some ways

currently it is used by neofs-node for server implementation (example), but this is not a burden: the node make codegen for itself

i even like how it sounds: client codegen - for the client implementation, server - for the server. They may be parameterized more specific and flexible. Although now the node uses the SDK client to access other nodes

roman-khimov commented 4 months ago

I thought api matches well with the names of the corresponding repository and protocol

That's true for us, but most people outside don't know what neofs-api is. They just take the SDK and see some api there. If it'd be raw or rawproto it'd be easier to digest.

currently it is used by neofs-node for server implementation

Let's have it exposed for now, perform the transition to the new SDK and then see what uses of api we have case-by-case. Duplicating generated code isn't elegant either, if these definitions have some real use outside then we can have them exported.