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: Remove redundant txn param from fetcher start #1635

Merged
merged 1 commit into from
Jul 17, 2023
Merged

refactor: Remove redundant txn param from fetcher start #1635

merged 1 commit into from
Jul 17, 2023

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1623

Description

Removes the redundant txn param from fetcher start as it is always the same value as fetcher.Init

@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 code quality Related to improving code quality action/no-benchmark Skips the action that runs the benchmark. labels Jul 13, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.6 milestone Jul 13, 2023
@AndrewSisley AndrewSisley requested a review from a team July 13, 2023 17:44
@AndrewSisley AndrewSisley self-assigned this Jul 13, 2023
@codecov
Copy link

codecov bot commented Jul 13, 2023 •
edited
Loading

Codecov Report

Patch coverage: 86.67% and project coverage change: +0.09 🎉

Comparison is base (f02e2cd) 75.54% compared to head (d5e0642) 75.62%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1635      +/-   ##
===========================================
+ Coverage    75.54%   75.62%   +0.09%     
===========================================
  Files          200      200              
  Lines        20808    20807       -1     
===========================================
+ Hits         15718    15735      +17     
+ Misses        4006     3994      -12     
+ Partials      1084     1078       -6
Flag Coverage Δ
all-tests 75.62% <86.67%> (+0.09%) ⬆️

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

Impacted Files Coverage Δ
db/fetcher/versioned.go 57.75% <78.95%> (ø)
db/collection_get.go 81.25% <100.00%> (ø)
db/collection_index.go 97.44% <100.00%> (ø)
db/fetcher/fetcher.go 72.91% <100.00%> (+0.07%) ⬆️
lens/fetcher.go 50.00% <100.00%> (-0.37%) ⬇️
planner/scan.go 88.32% <100.00%> (ø)

... and 5 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 f02e2cd...d5e0642. Read the comment docs.

fredcarle
fredcarle approved these changes Jul 17, 2023
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.

LGTM.

It seems that a more appropriate name for the PR would be Move txn from fetcher start to init don't you think?

@AndrewSisley
Copy link
Contributor Author

LGTM.

It seems that a more appropriate name for the PR would be Move txn from fetcher start to init don't you think?

I disagree with you on that :) Your suggestion suggests it doesnt already exist on init

@AndrewSisley
Remove reduntant txn param from fetcher start
d5e0642 
Is always the same value as fetcher.Init
@AndrewSisley AndrewSisley merged commit 8197795 into sourcenetwork:develop Jul 17, 2023
@AndrewSisley AndrewSisley deleted the 1623-rm-fetcher-start-txn branch July 17, 2023 20:21
@fredcarle
Copy link
Collaborator

LGTM.
It seems that a more appropriate name for the PR would be Move txn from fetcher start to init don't you think?

I disagree with you on that :) Your suggestion suggests it doesnt already exist on init

That's my point. It didn't.

@fredcarle
Copy link
Collaborator

fredcarle commented Jul 17, 2023 •
edited
Loading

I mean that it was part of the Init param but nothing was done with it.

@AndrewSisley
Copy link
Contributor Author

That's my point. It didn't.

I think you are mistaken. It was added to Init in an earlier Lens PR.

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@AndrewSisley
refactor: Remove redundant txn param from fetcher start (sourcenetwor…
5edc205 
…k#1635)

## Relevant issue(s)

Resolves sourcenetwork#1623

## Description

Removes the redundant txn param from fetcher start as it is always the
same value as fetcher.Init
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fredcarle fredcarle fredcarle approved these changes

Assignees

@AndrewSisley AndrewSisley

Labels
action/no-benchmark Skips the action that runs the benchmark. area/db-system Related to the core system related components of the DB code quality Related to improving code quality refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Milestone
DefraDB v0.6
Development

Successfully merging this pull request may close these issues.

Remove txn param from Fetcher.Start
2 participants
@AndrewSisley @fredcarle