Merge lp:~jamesh/storm/sqlite-sync into lp:storm
Proposed by
James Henstridge
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Merge reported by: | James Henstridge | ||||
| Merged at revision: | 251 | ||||
| Proposed branch: | lp:~jamesh/storm/sqlite-sync | ||||
| Merge into: | lp:storm | ||||
| To merge this branch: | bzr merge lp:~jamesh/storm/sqlite-sync | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Gustavo Niemeyer | Approve | ||
|
Review via email:
|
|||
To post a comment you must log in.
Revision history for this message
| Richard Boulton (richardboulton) wrote | # |
I did a quick visual inspection of the code - the implementation looks fine (though I've not tested it), and the feature looks useful. Turning off synchronisation for the (non time-critical) tests seems a reasonable thing to do to speed them up since it doesn't matter if the databases are lost due to power failure...
Maybe it's just me, but the lack of comments (or, more precisely, doccomments) bugs me. This seems to be the storm coding style, though... In particular, I think the SQLite.__init__ method could do with a doccomment describing the URI format, which would describe this new option (and also the existing "timeout" option, and the way to create an in-memory database). Or, if the URI format is described elsewhere, the doccomment could give a pointer to that documentation.
(It would be nice if what documentation there is was kept in bzr, so that small changes like this could be reviewed together with the documentation for the change, but that's not particular to this patch, of course.)
Certainly, the new feature should be documented somewhere - write the documentation at the same time as the code, and it becomes much less of a burden. Or so I say to myself every time I come across code I've written and not documented at the time!