Skip to content

Remove unused funcs in tx.go - #3128

Merged
N-o-Z merged 1 commit into
treeverse:masterfrom
codeaucafe:fix/datadavd/remove-unused-funcs-in-tx-3078
Mar 28, 2022
Merged

Remove unused funcs in tx.go#3128
N-o-Z merged 1 commit into
treeverse:masterfrom
codeaucafe:fix/datadavd/remove-unused-funcs-in-tx-3078

Conversation

@codeaucafe

@codeaucafe codeaucafe commented Mar 27, 2022

Copy link
Copy Markdown
Contributor

Remove unused functions in tx.go

This PR closes #3078
@codeaucafe

codeaucafe commented Mar 27, 2022

Copy link
Copy Markdown
Contributor Author

FYI I made this PR pretty quick, but after reviewing code a bit more, I'm wondering if there is a reason this has stayed in the code base but hasn't been used yet. Do we actually want to remove it? Or are the foreseen places or circumstances where these functions will be used in the future?
@itaiad200
itaiad200 requested a review from N-o-Z March 28, 2022 07:20
@N-o-Z

N-o-Z commented Mar 28, 2022

Copy link
Copy Markdown
Contributor

@DataDavD As best practices we try not to leave unused code in the repository. Worst case - we will re add it when needed
N-o-Z
N-o-Z approved these changes Mar 28, 2022
View reviewed changes

@N-o-Z N-o-Z left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Thanks!
@N-o-Z N-o-Z added contributor exclude-changelog PR description should not be included in next release changelog labels Mar 28, 2022
@N-o-Z

N-o-Z commented Mar 28, 2022

Copy link
Copy Markdown
Contributor

@DataDavD All checks have passed - you can press the merge button to merge the changes
@codeaucafe

codeaucafe commented Mar 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Sounds good. I figured that was the case, but just wanted to be sure since I'm not too knowledgeable about the history and roadmap of lakeFS. Thought it was fine since its in the git history anyways 😄. Thank you for the quick review!

@DataDavD As best practices we try not to leave unused code in the repository. Worst case - we will re add it when needed
@codeaucafe

codeaucafe commented Mar 28, 2022

Copy link
Copy Markdown
Contributor Author

Great! Unfortunately, it appears I don't have the perms to Merge 😞. Are you able to merge for me when you get a chance? Thanks in advance!

@DataDavD All checks have passed - you can press the merge button to merge the changes
@N-o-Z
N-o-Z merged commit 13088de into treeverse:master Mar 28, 2022
@N-o-Z

N-o-Z commented Mar 28, 2022

Copy link
Copy Markdown
Contributor

Done, thanks!
@arielshaqed

arielshaqed commented Mar 28, 2022

Copy link
Copy Markdown
Contributor

Hi @DataDavD ,

Thanks for this PR!

Some background / context / legends and origin stories: we used to use PostgreSQL a lot more. At that time selecting appropriate consistency models made sense.

With #2466 coming up, we have been removing usage of RDBMS features. So we no longer need these functions, and (I hope) there should be no use for them in the near future. So IMO you're spot-on that they meeded to go!
@codeaucafe

codeaucafe commented Mar 28, 2022

Copy link
Copy Markdown
Contributor Author

@arielshaqed thanks for the great detailed response/info. It's very helpful.

That is one of the many things I love about lakeFS/treeverse engineers like you. Y'all are always willing to help out and provide good explanations for everything to make it easy on us new contributors to understand why things have been done, are being done, and will be done.

Also, very excited to hear about #2466 !!!!! 🥳

Thanks again!

Best,
David
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor exclude-changelog PR description should not be included in next release changelog

3 participants

@codeaucafe @N-o-Z @arielshaqed