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

Remove unused funcs in tx.go #3128

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Remove unused funcs in tx.go #3128

merged 1 commit into from
Mar 28, 2022

Conversation

DataDavD
Copy link
Contributor

Remove unused functions in tx.go

This PR closes #3078

@DataDavD
Copy link
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
Copy link
Member

N-o-Z commented Mar 28, 2022

@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
Copy link
Member

@N-o-Z N-o-Z 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. Thanks!

@N-o-Z N-o-Z added the exclude-changelog PR description should not be included in next release changelog label Mar 28, 2022
@N-o-Z
Copy link
Member

N-o-Z commented Mar 28, 2022

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

@DataDavD
Copy link
Contributor Author

DataDavD commented Mar 28, 2022 •
edited
Loading

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

@DataDavD
Copy link
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
Copy link
Member

N-o-Z commented Mar 28, 2022

Done, thanks!

@arielshaqed
Copy link
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!

@DataDavD
Copy link
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
Reviewers

@N-o-Z N-o-Z N-o-Z approved these changes

Assignees
No one assigned
Labels
contributor exclude-changelog PR description should not be included in next release changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove unused functions in tx.go
3 participants
@DataDavD @N-o-Z @arielshaqed