search

OpenHospitalityNetwork / fedi-trustroots

Next generation federated hospitality exchange platform
https://openHospitality.network
GNU Affero General Public License v3.0
23 stars 3 forks source link

Problem: Commented code that can be removed #82

Open weex opened 2 years ago

weex commented 2 years ago

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 commented 2 years ago

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

weex commented 2 years ago

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 commented 2 years ago

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.