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

feat: Remove IsObjectArray #2859

Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #2356

Description

Removes IsObjectArray.

As well as causing confused code, and an overly-large interface, where their are multiple overlapping ways to check the same thing, this func also dirties the solution for #2493 and #2619.

PR is marked as a feature as it is a public change, could be called 'fix', I think it is a bit vague here give me a shout if you have a strong preference.

@AndrewSisley AndrewSisley added feature New feature or request area/schema Related to the schema system code quality Related to improving code quality labels Jul 23, 2024
@AndrewSisley AndrewSisley added this to the DefraDB v0.13 milestone Jul 23, 2024
@AndrewSisley AndrewSisley requested a review from a team July 23, 2024 18:06
@AndrewSisley AndrewSisley self-assigned this Jul 23, 2024
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.41%. Comparing base (25a3063) to head (9cb75c8).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2859      +/-   ##
===========================================
+ Coverage    79.38%   79.41%   +0.03%     
===========================================
  Files          323      323              
  Lines        24693    24683      -10     
===========================================
- Hits         19602    19601       -1     
+ Misses        3687     3680       -7     
+ Partials      1404     1402       -2     
Flag Coverage Δ
all-tests 79.41% <91.30%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
client/document.go 72.02% <100.00%> (+0.22%) ⬆️
client/schema_field_description.go 63.16% <ø> (-2.42%) ⬇️
internal/db/definition_validation.go 95.12% <100.00%> (ø)
internal/planner/mapper/mapper.go 89.55% <100.00%> (ø)
internal/request/graphql/schema/collection.go 90.35% <100.00%> (ø)
internal/request/graphql/schema/generate.go 87.95% <100.00%> (+0.23%) ⬆️
internal/db/collection.go 73.65% <0.00%> (-0.40%) ⬇️
internal/planner/type_join.go 81.23% <80.00%> (ø)

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25a3063...9cb75c8. Read the comment docs.

Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

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

Looks good! Code is much easier to read now.

ttype, ok = g.manager.schema.TypeMap()[field.Kind.Underlying()]
if !ok {
return nil, NewErrTypeNotFound(field.Kind.Underlying())
}
Copy link
Member

@nasdf nasdf Jul 23, 2024

Choose a reason for hiding this comment

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

nitpick: minor simplification

if field.Kind.IsObject() {
	var ok bool
	ttype, ok = g.manager.schema.TypeMap()[field.Kind.Underlying()]
	if !ok {
		return nil, NewErrTypeNotFound(field.Kind.Underlying())
	}
	if field.Kind.IsArray() {
		ttype = gql.NewList(ttype)
	}
}

Copy link
Contributor Author

@AndrewSisley AndrewSisley Jul 23, 2024

Choose a reason for hiding this comment

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

Good spot, thanks :)

  • generate.go simplification

@AndrewSisley AndrewSisley merged commit dc76eba into sourcenetwork:develop Jul 23, 2024
41 of 42 checks passed
@AndrewSisley AndrewSisley deleted the 2356-rm-is-object-array branch July 23, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/schema Related to the schema system code quality Related to improving code quality feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IsObject shouldn't be true for an array of objects
2 participants