-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add labels support to lianad #605
Conversation
88b61ef
to
f3b30e3
Compare
a7d0378
to
b622203
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments. I like the KISS approach you took. I implemented a functional test here, although there isn't much state consistency to be testing, feel free to cherry-pick it: darosior@af19dd3.
205cce5
to
ddb3fc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This looks good to me apart from a couple things in comments.
deb7cbf
to
ed45ccc
Compare
ed45ccc
to
d772297
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had forgotten to publish my review...
155cae1
to
260443a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me but the two mistakes in the error messages. Since i was requesting some changes anyways i added a couple nits on the documentation.
// After Liana 1.1 we upgraded the schema to add the labels table. | ||
fn migrate_v2_to_v3(conn: &mut rusqlite::Connection) -> Result<(), SqliteDbError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this using the test_migration
functional test.
260443a
to
204c160
Compare
ACK 204c160 |
No description provided.