search

hapostgres / pg_auto_failover

Postgres extension and service for automated failover and high-availability
Other
1.12k stars 115 forks source link

Refactor the same error used in 6-7 different places #806

Closed SaitTalhaNisanci closed 2 years ago

SaitTalhaNisanci commented 3 years ago

Now, I don't think such refactoring are very important. Which means I will accept that we merge the PR when it's ready, but also that I can't be too excited about those code changes.

The main reason for this refactor was that it seems that we want to give the same error message in those 7 places, so when we want to update one of the error messages, we will need to update all of the places. I thought that it is better that we update only one place, what downside do you see with using one place? Is that about "the error is not visible?"? If you can give the reasoning, I am okay with not merging this PR as well

DimCitus commented 3 years ago

The main reason for this refactor was that it seems that we want to give the same error message in those 7 places, so when we want to update one of the error messages, we will need to update all of the places. I thought that it is better that we update only one place, what downside do you see with using one place? Is that about "the error is not visible?"? If you can give the reasoning, I am okay with not merging this PR as well

Well if you spend the efforts to refactor that way, then I think it's worth it, because it's now done and as you say there is something nice about having the error message centralised in one place. That said when learning the code you now have one more indirection to learn, and if we want to change/enrich the error message in one place we need to think about changing all places or just that one, etc.

The win is not obvious, and the effort is real. But now that you've done the efforts...