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

Problem: Commented code that can be removed #82

Open
weex opened this issue Oct 10, 2021 · 3 comments
Open

Problem: Commented code that can be removed #82

weex opened this issue Oct 10, 2021 · 3 comments

Comments

@weex
Copy link

weex commented Oct 10, 2021

Just merged a couple PRs (#54 and #57) which left behind some commented code that we'd like to avoid to keep the codebase clear and concise.

@mariha
Copy link
Member

mariha commented Oct 10, 2021

I guess we may prefer to hide that code behind feature toggles and minimize changes to avoid potential conflicts when updating from upstream?

@weex
Copy link
Author

weex commented Oct 12, 2021

In my experience GitHub's fetch from upstream is pretty smart. I mainly see conflicts when the same line or lines are changed in both places since the last fetch and at those times, resolving is generally simple. Feature flags are useful of course, but I don't feel they're necessary to keep the fetches working smoothly.

My point is the code should be kept if it is useful for this fork. The removed code can be added easily if we experience a Problem.

@mariha
Copy link
Member

mariha commented Oct 12, 2021

Yea, I think this is exactly how updating and merging changes works, that git applies patches with lines changed one patch at a time from both branches and if there were changes in the same line in any two patches it results in a conflict, sometimes there may be many conflicts in the same line during one merge if some piece of code was changed a lot on both sides.

As long as Trustroots does not change anything in the commented lines of code, we shouldn't get conflicts. If we removed this piece of code, every time they change something, we'll need to resolve a conflict.

Whenever possible (everywhere?) I'd have added even the simplest feature toggle, a local variable with hardcoded true/false value, local to the place it is used, and then uncomment that code. It will be unreachable until the toggle is on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants