Document SchemaManager #2925
Document SchemaManager #2925
Conversation
|
@uklotzde better now? :) |
| if (backwardsCompatibleVersion.isNull() || !ok) { | ||
| // rryan 11/2010 We just added the backwards compatible flags, and some |
There was a problem hiding this comment.
Why did your removed this part??
There was a problem hiding this comment.
It's an exception for an ancient version we definitely don't support anymore.
There was a problem hiding this comment.
@Holzhaus Why has this been merged? It is not possible to revoke a database version once it has been released. All exceptions in the upgrade path must be preserved forever.
There was a problem hiding this comment.
This is why I asked and expressed my concerns about this removal.
There was a problem hiding this comment.
@xerus2000 Unfortunately you not only documented the SchemaManager as advertised but slipped in functional changes. That's the main reason why I don't trust your "cleanup" PRs that might contain some hidden changes as this one.
There was a problem hiding this comment.
This would only affect people on schema version 7 and want to downgrade to schema version 3 which isn't possible with the current Mixxx anyway, right? So you'd need to grab an old version of Mixxx, and in that case the code is still there. Or am I missing something?
There was a problem hiding this comment.
Anyway, I'll revert that change.
There was a problem hiding this comment.
I am also not sure. Whoever is removing this code should is in charge to explain in detail why this removal cannot affect any users of Mixxx. I'm not planning to remove this code, so I don't need to think about it. As simple as that.
There was a problem hiding this comment.
This only affected developers of mixxx who were using mixxx in a narrow timespan where schema version 7 was released without a min_compatible flag. Every version of mixxx you can get now includes the min_compatible flag in version 7, so this is superfluous. It is just confusing when reading the code and carries no use for anyone anymore. It is dead code.
src/database/schemamanager.h
Outdated
| bool isBackwardsCompatibleWithVersion(int targetVersion) const; | ||
|
|
||
| /// Does nothing if the versions are incompatible or the targetVersion is |
There was a problem hiding this comment.
"Tries to update the database schema to targetVersion. Pending changes are rolled back upon failure."
There was a problem hiding this comment.
I thought you didn't want to repeat the signature in words, and isn't that the first sentence?
There was a problem hiding this comment.
Do whatever you think is right. I am out.
src/database/schemamanager.h
Outdated
| @@ -26,8 +29,15 @@ class SchemaManager { | |||
| return m_currentVersion; | |||
| } | |||
|
|
|||
| /// If the current version has no backwards compatibility info, it is | |||
There was a problem hiding this comment.
Please omit the implementation details: "Checks if the current schema version is backwards compatible with the given targetVersion."
There was a problem hiding this comment.
But this "implementation detail" is extremely important, especially if you write a new schema revision. I wouldn't consider this a "detail", rather an essential information about the logic.
|
I am not reviewing this PR any longer. Maybe I am biased. Someone else may take over. |
|
Sorry if I am annoying you @uklotzde - I have actually implemented the changes you requested as far as I am aware, I am just trying to understand them as well :) |
|
@uklotzde Okay to merge or do we need more changes? |
While working on understanding it, I wrote some documentation and tried to reduce in-code comments.
This also serves as feedback whether I understood it correctly.