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

fix: Rename commit field input arg to fieldId #1460

Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1452

Description

Renames commit field input arg to fieldId, to match the field fieldId. Includes the explain output.

@AndrewSisley AndrewSisley added area/query Related to the query component code quality Related to improving code quality action/no-benchmark Skips the action that runs the benchmark. labels May 5, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.5.1 milestone May 5, 2023
@AndrewSisley AndrewSisley requested a review from a team May 5, 2023 21:41
@AndrewSisley AndrewSisley self-assigned this May 5, 2023
@@ -20,6 +20,7 @@ const (
DocKey = "dockey"
DocKeys = "dockeys"
FieldName = "field"
FieldIDName = "fieldId"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: new property required, as some references to FieldName were actually for field in stuff like aggregates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There was already FieldIDFieldName bellow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FieldIDFieldName is the field name, not the input arg name.

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #1460 (dcab186) into develop (c643beb) will not change coverage.
The diff coverage is 71.42%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1460   +/-   ##
========================================
  Coverage    72.22%   72.22%           
========================================
  Files          185      185           
  Lines        18224    18224           
========================================
  Hits         13162    13162           
  Misses        4026     4026           
  Partials      1036     1036           
Impacted Files Coverage Δ
client/request/commit.go 100.00% <ø> (ø)
planner/mapper/commitSelect.go 0.00% <0.00%> (ø)
planner/commit.go 78.08% <75.00%> (ø)
planner/mapper/mapper.go 86.99% <100.00%> (+0.67%) ⬆️
request/graphql/parser/commit.go 78.57% <100.00%> (ø)

... and 4 files with indirect coverage changes

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

I have a few thoughts I'd like to discuss before approving.

Cid immutable.Option[string]
Depth immutable.Option[uint64]
DocKey immutable.Option[string]
FieldIDName immutable.Option[string]
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: I'm not sure FieldIDName makes sense here. FiedID seemed more appropriate.

Copy link
Contributor Author

@AndrewSisley AndrewSisley May 8, 2023

Choose a reason for hiding this comment

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

Cheers, I think you are right and this is probably either a find and replace or auto-pilot induced mistake :) Will sort these out.

  • rename mapper.Commit prop

@@ -23,7 +23,7 @@ type CommitSelect struct {
DocKey immutable.Option[string]

// The field for which commits have been requested.
FieldName immutable.Option[string]
FieldIDName immutable.Option[string]
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: Same as above.

Copy link
Contributor Author

@AndrewSisley AndrewSisley May 8, 2023

Choose a reason for hiding this comment

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

Cheers, I think you are right and this is probably either a find and replace or auto-pilot induced mistake :) Will sort these out.

  • rename commitSelect prop

@@ -20,6 +20,7 @@ const (
DocKey = "dockey"
DocKeys = "dockeys"
FieldName = "field"
FieldIDName = "fieldId"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There was already FieldIDFieldName bellow.

@@ -37,9 +37,9 @@ func parseCommitSelect(schema gql.Schema, parent *gql.Object, field *ast.Field)
} else if prop == request.Cid {
raw := argument.Value.(*ast.StringValue)
commit.Cid = immutable.Some(raw.Value)
} else if prop == request.FieldName {
} else if prop == request.FieldIDName {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought(out-of-scope): I'm noticing that the prop is being compared to field names and we have the proper constants to make it a more visually descriptive conditional.

LinksFieldName           = "links"
HeightFieldName          = "height"
CidFieldName             = "cid"
DockeyFieldName          = "dockey"
CollectionIDFieldName    = "collectionID"
SchemaVersionIDFieldName = "schemaVersionId"
FieldNameFieldName       = "fieldName"
FieldIDFieldName         = "fieldId"
DeltaFieldName           = "delta"

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Thanks for the change Andy!

@AndrewSisley AndrewSisley force-pushed the 1452-commits-field-id-rename branch from 47ae50e to dcab186 Compare May 8, 2023 22:56
@AndrewSisley AndrewSisley merged commit 7575209 into sourcenetwork:develop May 8, 2023
@AndrewSisley AndrewSisley deleted the 1452-commits-field-id-rename branch May 8, 2023 23:10
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1452 

## Description

Renames commit `field` input arg to `fieldId`, to match the field
`fieldId`. Includes the explain output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/query Related to the query component code quality Related to improving code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

commits input arg is call field, but the corresponding field is call fieldId
3 participants