-
Notifications
You must be signed in to change notification settings - Fork 110
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
feat: support sqlite #185
base: master
Are you sure you want to change the base?
feat: support sqlite #185
Conversation
@christoshadjiaslanis please take a look. If you want to take it over it's fine by me 😃 |
@rasviitanen thank you for this it looks great! I'll happily take it over the line from here. I think the deterministic sampling is a prerequisite for the testing harness (if we stick to the golden master strategy we have for other datasources). Regarding the bounds - I think they're breaking some tests. I'm not sure I understand why this felt unnatural - perhaps you could elaborate? Regarding generating into an empty directory - I think that's a great idea so thank you! |
@christoshadjiaslanis Thanks! I guess the bounds issue originates from synth generating an empty bounds set |
@@ -137,9 +138,9 @@ impl Cli { | |||
self.store.save_collection_path(&path, collection, content)?; | |||
Ok(()) | |||
} | |||
} else if self.store.ns_exists(&path) { | |||
} else if self.store.ns_exists(&path) && !self.store.ns_is_empty_dir(&path) { |
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.
This should be a separate PR.
Adds support for sqlite.
closes: #161
Todo
test harness
- we should proabably create a test harness atsynth/testing_harness/sqlite
. But here we don't have to run docker, so question is if we want something else?random
doesn't support a seed.Other
low=Bound::Included(1)
andhigh=Bound::Excluded(1)
. To me it feels natural that this range includes a number1
, so I added some code for this [commit !8ba84fa]. It also includes the number if you havelow=Bound::Excluded(1)
andhigh=Bound::Included(1)
, I am not equally sure about this case though.I can drop these two commits if they are not desired.