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.

Sign up for GitHub

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

Jump to bottom

refactor: Add NormalValue #2404

Merged
merged 25 commits into from
Mar 28, 2024
Merged

refactor: Add NormalValue #2404

merged 25 commits into from
Mar 28, 2024

Conversation

islamaliev
Copy link
Contributor

Relevant issue(s)

Resolves #2403

Description

This change introduces NormalValue type that carries a standard (or normal) value that doesn't need to be type-asserted with all possible types.

@islamaliev islamaliev self-assigned this Mar 12, 2024
@islamaliev islamaliev added the refactor This issue specific to or requires *notable* refactoring of existing codebases and components label Mar 12, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.52292% with 253 lines in your changes are missing coverage. Please review.

Project coverage is 75.07%. Comparing base (a2591f8) to head (1163edc).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2404      +/-   ##
===========================================
+ Coverage    74.97%   75.07%   +0.10%     
===========================================
  Files          268      278      +10     
  Lines        25984    26945     +961     
===========================================
+ Hits         19479    20227     +748     
- Misses        5178     5370     +192     
- Partials      1327     1348      +21
Flag Coverage Δ
all-tests 75.07% <78.52%> (+0.10%) ⬆️

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

Files Coverage Δ
client/errors.go 67.16% <100.00%> (+3.23%) ⬆️
client/normal_array.go 100.00% <100.00%> (ø)
client/normal_nil.go 100.00% <100.00%> (ø)
client/normal_nillable_array.go 100.00% <100.00%> (ø)
client/normal_nillable_array_of_nillables.go 100.00% <100.00%> (ø)
client/normal_nillable_scalar.go 100.00% <100.00%> (ø)
client/normal_scalar.go 100.00% <100.00%> (ø)
client/schema_field_description.go 70.50% <ø> (+7.91%) ⬆️
client/value.go 93.48% <100.00%> (+0.30%) ⬆️
core/encoding.go 74.63% <100.00%> (-1.74%) ⬇️
... and 11 more

... and 6 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 a2591f8...1163edc. Read the comment docs.

@islamaliev islamaliev force-pushed the refactor/normal_value branch from 09776f1 to d75ce10 Compare March 19, 2024 15:52
@islamaliev islamaliev requested review from fredcarle, AndrewSisley and a team March 19, 2024 16:17
@islamaliev islamaliev marked this pull request as ready for review March 19, 2024 16:26
@islamaliev islamaliev changed the title Add dedicated NormalValue struct refactor: Add NormalValue Mar 19, 2024
AndrewSisley
AndrewSisley requested changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

I'm still reviewing, but submitting a few suggestions now that may aid further review :)

client/normal_value.go Outdated Show resolved Hide resolved
client/normal_value.go Outdated Show resolved Hide resolved
client/normal_value.go Show resolved Hide resolved
client/normal_value.go Show resolved Hide resolved
fredcarle
fredcarle reviewed Mar 19, 2024
View reviewed 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.

Currently reviewing but I just want to point out that this PR is in conflict with #2414. #2414 should merge first and the conflict can be resolved here.

@islamaliev islamaliev force-pushed the refactor/normal_value branch from 501eefb to 06a3cd8 Compare March 21, 2024 10:45
AndrewSisley
AndrewSisley requested changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Submitting a handful of documentation requests now, interface looks good :)

client/normal_value.go Show resolved Hide resolved
client/normal_value.go Outdated Show resolved Hide resolved
client/normal_value.go Outdated Show resolved Hide resolved
client/normal_value.go Outdated Show resolved Hide resolved
client/normal_value.go Show resolved Hide resolved
client/normal_value.go Show resolved Hide resolved
client/normal_value.go Outdated Show resolved Hide resolved
AndrewSisley
AndrewSisley reviewed Mar 21, 2024
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley 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 :) Just a couple of small suggestions :)

client/normal_void.go Outdated Show resolved Hide resolved
client/value.go Show resolved Hide resolved
core/encoding.go Outdated Show resolved Hide resolved
core/encoding.go Outdated Show resolved Hide resolved
core/key_test.go Outdated Show resolved Hide resolved
client/normal_value.go Outdated Show resolved Hide resolved
db/fetcher/indexer_iterators.go Show resolved Hide resolved
client/normal_nil.go Show resolved Hide resolved
encoding/field_value.go Show resolved Hide resolved
client/normal_value.go Show resolved Hide resolved
@islamaliev islamaliev force-pushed the refactor/normal_value branch from a2841fc to 4bbe440 Compare March 22, 2024 13:29
@islamaliev islamaliev requested a review from AndrewSisley March 22, 2024 15:36
fredcarle
fredcarle reviewed Mar 22, 2024
View reviewed changes
client/normal_new.go Outdated Show resolved Hide resolved
fredcarle
fredcarle reviewed Mar 22, 2024
View reviewed changes
client/normal_new.go Outdated Show resolved Hide resolved
fredcarle
fredcarle reviewed Mar 22, 2024
View reviewed changes
client/normal_new.go Outdated Show resolved Hide resolved
fredcarle
fredcarle reviewed Mar 22, 2024
View reviewed changes
client/normal_util.go Outdated Show resolved Hide resolved
fredcarle
fredcarle reviewed Mar 22, 2024
View reviewed changes
client/normal_value.go Outdated Show resolved Hide resolved
fredcarle
fredcarle approved these changes Mar 22, 2024
View reviewed 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.

Approving and assuming you'll resolve all comments before merge :)

This is overall really nice and brings us to a place where we can easily add on if we need to as we go.

@islamaliev islamaliev force-pushed the refactor/normal_value branch from 9637109 to 8879157 Compare March 28, 2024 11:11
@islamaliev islamaliev requested a review from fredcarle March 28, 2024 11:34
AndrewSisley
AndrewSisley approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Islam!

@islamaliev islamaliev merged commit a2c3863 into sourcenetwork:develop Mar 28, 2024
31 checks passed
shahzadlone pushed a commit that referenced this pull request Apr 3, 2024
@islamaliev @shahzadlone
refactor: Add NormalValue (#2404)
d665370 
## Relevant issue(s)

Resolves #2403

## Description

This change introduces `NormalValue` type that carries a standard (or
normal) value that doesn't need to be type-asserted with all possible
types.
@fredcarle fredcarle added this to the DefraDB v0.11 milestone Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewSisley AndrewSisley AndrewSisley approved these changes

@fredcarle fredcarle Awaiting requested review from fredcarle

Assignees

@islamaliev islamaliev

Labels
refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Milestone
DefraDB v0.11
Development

Successfully merging this pull request may close these issues.

Have a single place in code that manages standard (or normal) types
3 participants
@islamaliev @fredcarle @AndrewSisley