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

refactor/Object transformer package #2671

Merged
merged 4 commits into from
Dec 15, 2023
Merged

Conversation

carpawell
Copy link
Member

No description provided.

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6151f15) 30.09% compared to head (d211e27) 30.09%.

Files Patch % Lines
pkg/services/object/get/util.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2671   +/-   ##
=======================================
  Coverage   30.09%   30.09%           
=======================================
  Files         406      406           
  Lines       30182    30182           
=======================================
  Hits         9083     9083           
  Misses      20319    20319           
  Partials      780      780           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@carpawell carpawell force-pushed the rm/node-side-slicer branch from 46bb381 to 671038c Compare December 7, 2023 19:43
@carpawell carpawell self-assigned this Dec 7, 2023
@carpawell carpawell changed the title wip refactor/Object transformer package Dec 7, 2023
@carpawell carpawell marked this pull request as ready for review December 7, 2023 19:44
@carpawell carpawell force-pushed the rm/node-side-slicer branch from 671038c to a5fbe5e Compare December 7, 2023 19:46
pkg/services/object/put/slice.go Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@ import (
"context"
"fmt"

"github.com/nspcc-dev/neofs-node/pkg/services/object_manager/transformer"
"github.com/nspcc-dev/neofs-node/pkg/services/object/internal"
Copy link
Member

Choose a reason for hiding this comment

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

Usually, internal/smth is used, because internal itself doesn't mean a lot.

Copy link
Member Author

Choose a reason for hiding this comment

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

can you suggest naming? object? target? objectarget?

Copy link
Member

Choose a reason for hiding this comment

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

It's strange enough we have a whole package just for this interface. To me it looks a lot like a part of pkg/services/object/put. But there it'd be internal. But then who needs an internal interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

#2671 (comment) describes where i saw this interface and why

part of it is used by the get service too. i wanted to see more usages but somehow we have so many WriteChunk and Write(object.Object) error (not the oi.Writer) so it does not look so useful sadly. but we can continue this refactor

Copy link
Member

Choose a reason for hiding this comment

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

get doesn't need it. It has the thing it needs already.

Copy link
Member Author

Choose a reason for hiding this comment

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

dont you like how HeaderWriter is included in the Target and how every object interface can be placed in a single common file? every header should be written with the same interface does not matter what service does it

@carpawell carpawell force-pushed the rm/node-side-slicer branch from 206c182 to 34189e9 Compare December 8, 2023 12:48
Not needed complication.

Signed-off-by: Pavel Karpy <[email protected]>
It stores ID only (but it was not always like this) but complicate the
interface.

Signed-off-by: Pavel Karpy <[email protected]>
`transformer` package is not needed anymore; SDK's `Slicer` is responsible for
the object payload limitations. Closes #2519.

Signed-off-by: Pavel Karpy <[email protected]>
Also, reuse it in the get service.

Signed-off-by: Pavel Karpy <[email protected]>
@roman-khimov roman-khimov merged commit 9072222 into master Dec 15, 2023
9 of 11 checks passed
@roman-khimov roman-khimov deleted the rm/node-side-slicer branch December 15, 2023 14:00
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.

3 participants