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

chore: Move Option and Enumerable to immutables #939

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

fredcarle
Copy link
Collaborator

Relevant issue(s)

Resolves #854
Resolves #853

Description

This PR adds no new code. It's simply moving or renaming things. Mainly, the Option and Enumerable types are moved the immutables package and the UpdateEvent is moved to the events package are renamed Update.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle fredcarle added code quality Related to improving code quality action/no-benchmark Skips the action that runs the benchmark. labels Nov 13, 2022
@fredcarle fredcarle added this to the DefraDB v0.4 milestone Nov 13, 2022
@fredcarle fredcarle requested a review from a team November 13, 2022 17:17
@fredcarle fredcarle self-assigned this Nov 13, 2022
@AndrewSisley
Copy link
Contributor

Enumerable might be leaving the defra repo entirely quite soon - not sure it is worth touching it here. This would also leave us with the immutables package containing only a doc.go and the options sub-package, which I'm not sure makes sense.

@fredcarle
Copy link
Collaborator Author

Enumerable might be leaving the defra repo entirely quite soon - not sure it is worth touching it here. This would also leave us with the immutables package containing only a doc.go and the options sub-package, which I'm not sure makes sense.

There wouldn't be an options sub-package. It stays at the root of immutables. It makes more sense to have it here than in client in my opinion.

@AndrewSisley
Copy link
Contributor

There wouldn't be an options sub-package. It stays at the root of immutables. It makes more sense to have it here than in client in my opinion.

You'd prefer an immutables package with a single type, than to either leave it where it is, or to move it to a sub-package in client?

And you'd prefer to move enumerable out here, only for it to be moved again pretty soonish?

@fredcarle
Copy link
Collaborator Author

You'd prefer an immutables package with a single type, than to either leave it where it is, or to move it to a sub-package in client?

Yes. I don't like that client is becoming a catch-all. And option.Option is weird and not idiomatic.

And you'd prefer to move enumerable out here, only for it to be moved again pretty soonish?

You mean it's going to be move to a separate repo? The work has been done so I don't think it's a big deal if we leave it there until it's taken out.

@AndrewSisley
Copy link
Contributor

You mean it's going to be move to a separate repo?

Yes, it is to be shared by lens and defra (or less-likely, but possible, removed and replaced from both by a 3rd party lib)

The rest :)

Okay not a problem, so long as we are aware and still happy

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.

AndrewSisley added a commit to sourcenetwork/immutable that referenced this pull request Nov 22, 2022
From sourcenetwork/defradb#939 with the licence headers stripped out
@fredcarle fredcarle force-pushed the fredcarle/refactor/I854-rework-client-types branch from 214cdef to 6d3065e Compare November 23, 2022 21:37
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #939 (54e8b6b) into develop (82e0d2f) will decrease coverage by 0.24%.
The diff coverage is 90.10%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #939      +/-   ##
===========================================
- Coverage    60.19%   59.94%   -0.25%     
===========================================
  Files          165      157       -8     
  Lines        17884    17685     -199     
===========================================
- Hits         10766    10602     -164     
+ Misses        6155     6129      -26     
+ Partials       963      954       -9     
Impacted Files Coverage Δ
client/request/commit.go 100.00% <ø> (ø)
client/request/mutation.go 100.00% <ø> (ø)
client/request/select.go 100.00% <ø> (ø)
db/fetcher/dag.go 58.62% <ø> (ø)
db/subscriptions.go 57.14% <ø> (ø)
events/events.go 100.00% <ø> (ø)
merkle/crdt/composite.go 80.00% <ø> (ø)
merkle/crdt/factory.go 85.71% <ø> (ø)
merkle/crdt/merklecrdt.go 47.05% <ø> (ø)
net/client.go 0.00% <0.00%> (ø)
... and 25 more

@fredcarle fredcarle force-pushed the fredcarle/refactor/I854-rework-client-types branch from 6d3065e to 54e8b6b Compare November 23, 2022 21:44
@fredcarle fredcarle merged commit 5e28187 into develop Nov 23, 2022
@fredcarle fredcarle deleted the fredcarle/refactor/I854-rework-client-types branch November 23, 2022 22:04
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Relevant issue(s)
Resolves sourcenetwork#854
Resolves sourcenetwork#853

Description
This PR adds no new code. It's simply moving or renaming things. Mainly, the Option and Enumerable types are moved the external immutable package and the UpdateEvent is moved to the events package are renamed Update.
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. code quality Related to improving code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move Enumerable out of core Move Option out of client
3 participants