refactor(robot-server): Add a comment to note a mistaken type in migration #12126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SyntaxColoring
changed the title
SyntaxColoring
commented
Feb 9, 2023
Comment on lines
+126
to
+130
| # NOTE: The column type of `STRING` here is mistaken. SQLite won't recognize it, | ||
| # so this column's type affinity will mistakenly default to `NUMERIC`. | ||
| # It should be `VARCHAR`, to match what SQLAlchemy uses when creating a new | ||
| # database from scratch. Fortunately, for this particular column, this inconsistency | ||
| # is harmless in practice because of SQLite's weak typing. |
There was a problem hiding this comment.
I think this is a signal that we need to either:
- Test that the "migrate from older database" path creates the exact same schema as the "create database from scratch" path.
- Completely remove the "create database from scratch path" in favor of a chain of migrations, so a mismatch is structurally impossible. Maybe Alembic can help with this.
Alembic would have helped with other things here, too. For example, it would have been proper to change the migrated columns' types from BLOB to VARCHAR, but I couldn't because the column type because SQLite makes that ridiculously difficult to do manually. Alembic, meanwhile, has specific support for this.
y3rsh
added a commit
that referenced
this pull request
Feb 13, 2023
* origin/edge: (31 commits) refactor(hardware): Update appropriate revision (#12130) fix(api): Fix accidental error on any PAPIv2.14 protocol (#12141) refactor(api): Allow homing after drop tip in engine core (#12124) fix(shared-data): remove newLocation and strategy from schemav6 (#12133) refactor(api): load pipette with useVirtualPipettes config option (#12117) chore(api): further speed up ot3controller tests and add profiling (#12132) docs(api): Settle in to leave Labware.default_magdeck_engage_height alone, for now (#12122) refactor(api): Improve error messages for JSONv6 and PAPIv2.14 protocols (#12131) refactor(robot-server): Note mistaken type in migration (#12126) chore(hardware): Remove docstring content checks (#12128) feat(hardware_control): Added firmware update mechanism for submodules (#12076) feat(app): remove feature flag for calibration dashboard and wizard updates (#12125) feat(app): Calibration dashboard wizard data invalidation (#12097) feat(hardware): support new revision values (#12111) feat(app): add Chip component (#12090) docs(api): Officially remove the `height` parameter of `MagneticModuleContext.engage()` (#12114) feat(system-server): add persistent UUID generation (#12123) feat(odd): add manual connection for a hidden network (#12033) feat(hardware): add progress output to subsystem firmware update process (#12059) refactor(api): Improve OT3 instrument calibration process (#11807) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR adds a comment to call out some tech debt in our migration code.
(This comment was originally part of #11561. I'm pulling it out here because we won't have a chance to merge that PR for a bit.)
Test Plan
None needed.
Review requests
Does this make sense to you?
Risk assessment
No risk. Comment only.