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

refactor: Simplify dag fetcher #913

Merged
merged 11 commits into from
Oct 25, 2022

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Quick refactor prior to starting #849

Description

Refactors the dag fetcher prior to starting #849 - should make working in this file a bit easier.

Is no need for this complication
Now we can see what it does, allowing for future improvements
Statefull prop doesn't really do anything
Instead of the end, plus init.  I find this much easier to read.
Is easier to read IMO, and allows for future refactorings
Simpler and easier to read
@AndrewSisley AndrewSisley added area/db-system Related to the core system related components of the DB refactor This issue specific to or requires *notable* refactoring of existing codebases and components action/no-benchmark Skips the action that runs the benchmark. labels Oct 24, 2022
@AndrewSisley AndrewSisley added this to the DefraDB v0.4 milestone Oct 24, 2022
@AndrewSisley AndrewSisley requested a review from a team October 24, 2022 18:24
@AndrewSisley AndrewSisley self-assigned this Oct 24, 2022
@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Merging #913 (11e3c4c) into develop (6ccae7f) will increase coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #913   +/-   ##
========================================
  Coverage    59.89%   59.90%           
========================================
  Files          159      159           
  Lines        17423    17391   -32     
========================================
- Hits         10436    10418   -18     
+ Misses        6053     6044    -9     
+ Partials       934      929    -5     
Impacted Files Coverage Δ
db/fetcher/dag.go 58.62% <77.77%> (+1.31%) ⬆️
planner/commit.go 84.35% <100.00%> (-0.09%) ⬇️

Is no reason for us to have to worry about this
db/fetcher/dag.go Outdated Show resolved Hide resolved
db/fetcher/dag.go Outdated Show resolved Hide resolved
@AndrewSisley AndrewSisley merged commit 3b53193 into develop Oct 25, 2022
@AndrewSisley AndrewSisley deleted the sisley/feat/I849-commit-collection-id branch October 25, 2022 20:55
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
* Remove commented out code from commitFetcher

* Remove cid from state

Is no need for this complication

* Remove unused type

* Unfactor func called once

Now we can see what it does, allowing for future improvements

* Remove unused return param

* Remove kvEnd

Statefull prop doesn't really do anything

* Call nextKey at the start of loop

Instead of the end, plus init.  I find this much easier to read.

* Unfactor single use function

Is easier to read IMO, and allows for future refactorings

* Remove iteration state

Simpler and easier to read

* Remove potential spans error

Is no reason for us to have to worry about this
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/db-system Related to the core system related components of the DB refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants