-
Notifications
You must be signed in to change notification settings - Fork 82
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
Don't create versions that would be identical to previous version #35
Don't create versions that would be identical to previous version #35
Conversation
@nathanielobrown thanks for your contribution! |
@simoneb, I'm happy you like it! Maybe we should update the README so it no longer says:
|
@washingtonsoares can you take care of this please? |
@nathanielobrown Actually I think this section still makes sense. Because even after the change you proposed, by default this extension will keep creating a new record in the history with each update regardless of whether there was in fact a change, unless we passed the fourth parameter to disable it (I believe that this PR will only work if we are dealing with this fourth parameter, right?) |
@washingtonsoares, you are right, my bad. I quoted the wrong sentence from the this section. I think this sentence should be removed or modified:
You are right, you must pass the fourth parameter as TRUE for this to be active |
@simoneb I'm late to the party, but I think that inclusion of this change in the latest release should have triggered a major as this change is not backward compatible. |
We have discussed this extensively and while it may be true that this should have been considered as a breaking change, for most practical purposes the impact on existing users is extremely limited because it addresses a very specific scenario, that of the source and history tables having a different schema. Nevertheless, because this versioning is quite loose (i.e there's no automatic dependency upgrade or anything like that), we didn't think too much about semver. Thanks for bringing it up though |
I would find it much more useful if the "Ignore updates without actual change" would apply to no actual change to the version history, rather than no change to the source table. Why?
This is my first time editing plpgsql and it took me a while so I'm kinda opening this for feedback and to start a discussion. Even if this shouldn't be merged (it's backwards incompatible, after all) I would really appreciate any feedback as I'm going to go ahead and use it myself 😬. Maybe this should be controlled by a different option, maybe there's a better way to do this check... just throwing this out there, not saying it should be merged 🙂