Refactorings #223

Merged
fyrchik merged 1 commits from dstepanov-yadro/frostfs-node:refactoring/object-3060-remain into master 2023-07-26 21:07:57 +00:00

Resolve linters

Resolve linters
dstepanov-yadro force-pushed refactoring/object-3060-remain from 8bb258de5b to 9e397d9b18 2023-04-06 13:32:02 +00:00 Compare
dstepanov-yadro changed title from WIP: Refactorings to Refactorings 2023-04-06 13:32:38 +00:00
dstepanov-yadro requested review from storage-core-committers 2023-04-06 13:32:45 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-04-06 13:32:45 +00:00
dstepanov-yadro force-pushed refactoring/object-3060-remain from 9e397d9b18 to 3c4a4c3a38 2023-04-06 14:40:55 +00:00 Compare
ale64bit requested changes 2023-04-10 09:40:12 +00:00
@ -213,106 +211,124 @@ func (v *FormatValidator) ValidateContent(o *object.Object) (ContentMeta, error)
case object.TypeRegular:
Collaborator

remove this, since a default case was added?

remove this, since a `default` case was added?
Poster
Collaborator

fixed

fixed
ale64bit marked this conversation as resolved
@ -216,2 +214,2 @@
if len(o.Payload()) == 0 {
return ContentMeta{}, fmt.Errorf("(%T) empty payload in tombstone", v)
if err := v.fillAndValidateTombstoneMeta(o, &meta); err != nil {
return ContentMeta{}, err
Collaborator

Maybe expand the error? fmt.Errorf("validating tombstone: %v", err)
Since the new fillAndValidateXXX functions do not always include info about the type. Same in the other cases below.

Maybe expand the error? `fmt.Errorf("validating tombstone: %v", err)` Since the new `fillAndValidateXXX` functions do not always include info about the type. Same in the other cases below.
Poster
Collaborator

As I understood @fyrchik comment (#223 (comment)), there is no need to add new functionality, since this is refactoring.

As I understood @fyrchik comment (https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/223#issuecomment-5164), there is no need to add new functionality, since this is refactoring.
ale64bit marked this conversation as resolved
@ -245,3 +237,1 @@
if len(o.Payload()) == 0 {
return ContentMeta{}, fmt.Errorf("(%T) empty payload in SG", v)
}
_, ok := o.ContainerID()
Collaborator

do we have some rule to not one-line these? (same below)

if _, ok := o.ContainerID(); !ok {
    (...)
}

this is not introduced by this PR, but seems to happen a lot in the codebase. Not sure what's the benefit of polluting the namespace when there's no need.

do we have some rule to not one-line these? (same below) ``` if _, ok := o.ContainerID(); !ok { (...) } ``` this is not introduced by this PR, but seems to happen a lot in the codebase. Not sure what's the benefit of polluting the namespace when there's no need.
Poster
Collaborator

fixed

fixed
ale64bit marked this conversation as resolved
@ -279,2 +280,2 @@
return ContentMeta{}, errors.New("missing container")
}
if err := sg.Unmarshal(o.Payload()); err != nil {
return fmt.Errorf("(%T) could not unmarshal SG content: %w", v, err)
Collaborator

can we avoid the acronym 'SG' here?

can we avoid the acronym 'SG' here?

This is a refactoring, let's not change any messages.

This is a refactoring, let's not change any messages.
ale64bit marked this conversation as resolved
@ -313,3 +327,3 @@
}
return meta, nil
idList := tombstone.Members()
Collaborator

why the local variable?

why the local variable?
Poster
Collaborator

fixed

fixed
ale64bit marked this conversation as resolved
dstepanov-yadro force-pushed refactoring/object-3060-remain from 3c4a4c3a38 to e1765dc121 2023-04-10 10:56:32 +00:00 Compare
ale64bit approved these changes 2023-04-10 11:02:51 +00:00
dstepanov-yadro force-pushed refactoring/object-3060-remain from e1765dc121 to 2ce721d52d 2023-04-10 11:11:13 +00:00 Compare
dstepanov-yadro force-pushed refactoring/object-3060-remain from 2ce721d52d to 2c07f831c7 2023-04-10 11:17:31 +00:00 Compare
fyrchik approved these changes 2023-04-10 15:16:55 +00:00
fyrchik merged commit 2c07f831c7 into master 2023-04-10 15:17:00 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#223
There is no content yet.