Issue 6494142: whoosh create/destroy storage api
|
Created:
11 years, 7 months ago by Thomas.J.Waldmann Modified:
11 years, 6 months ago Reviewers:
matt2, thomas.j.waldmann, thomas.j.waldmann Visibility:
Public. |
Patch Set 1 #
Total comments: 21
MessagesTotal messages: 4
hi matt, i think it is easier to discuss such stuff on codereview, please have a look. if you change the code, you can just call "python upload.py -i 6494142" in your workdir to update this issue with your uncommitted changes. If you like codereview, I can also upload my changes here first, so you can review them. Cheers, Thomas http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py File src/whoosh/filedb/filestore.py (right): http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:64: st = FileStorage(dirname) do you really want to still document this way? you know, people always use the first example they can find... http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:68: set-up of the storage object. Ffor example, ``FileStorage.create()`` creates typo Ffor http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:103: """ somehow this way (using a class method that returns the storage object after doing whatever needed to create the storage) is somehow duplicating the arguments needed for the class constructor. So, if i want to have this flexibly configured in moin, would I have to configure both arguments for create() and for __init__(), just because they COULD differ? I think it might be easier to have __init__ not do much, except getting and storing all the params needed (esp. it shall not check if the storage already exists). Then have a create() instance method that creates the storage and a destroy() instance method that destroys it. If __init__() gets all params needed, create() and destroy() could live without any params, right? http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:113: """ see above. http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:235: """Returns the last-modified time (as a ctime-type integer) of the ctime-type? do you mean a UNIX timestamp? http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:291: """ btw, how do you think about context managers for such stuff, so one can use "with"? http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:402: return "%s(%s)" % (self.__class__.__name__, repr(self.folder)) "%s(%r)" and no repr()? http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:411: directory doesn't exist, pass ``makedir=False``:: you don't use makdir in the code below. also, using makedir=False would be somehow strange with the current code, as __init__() checks for the directory anyway (which I think is no good, see my comments above). http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:435: if not os.path.exists(dirpath): why not just call makedirs directly without checking first? http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:440: e = sys.exc_info()[1] why not just: except IOError, e: http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:451: raise e i think this all could be much easier with some assertions: when create() is called, there MUST NOT be anything at that place (not a file, not directory, not even an empty directory). this is the same state as destroy() should leave behind: nothing left there, so i think this would be quite clean and symmetric.
Sign in to reply to this message.
I'm not sure if this is using Reitveld the right way, but I've replied here and also applied fixes in changeset 49468947db7e. http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py File src/whoosh/filedb/filestore.py (right): http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:68: set-up of the storage object. Ffor example, ``FileStorage.create()`` creates On 2012/09/15 14:00:30, ThomasJWaldmann wrote: > typo Ffor Thanks :) http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:103: """ On 2012/09/15 14:00:30, ThomasJWaldmann wrote: > somehow this way (using a class method that returns the storage object after > doing whatever needed to create the storage) is somehow duplicating the > arguments needed for the class constructor. > > So, if i want to have this flexibly configured in moin, would I have to > configure both arguments for create() and for __init__(), just because they > COULD differ? > > I think it might be easier to have __init__ not do much, except getting and > storing all the params needed (esp. it shall not check if the storage already > exists). Then have a create() instance method that creates the storage and a > destroy() instance method that destroys it. > > If __init__() gets all params needed, create() and destroy() could live without > any params, right? Ah, yeah, that's a much better idea. http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:235: """Returns the last-modified time (as a ctime-type integer) of the On 2012/09/15 14:00:30, ThomasJWaldmann wrote: > ctime-type? do you mean a UNIX timestamp? I've made this clearer. http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:291: """ On 2012/09/15 14:00:30, ThomasJWaldmann wrote: > btw, how do you think about context managers for such stuff, so one can use > "with"? That doesn't make much sense to me, since an index is meant to be persistent, not something you create for a few operations and the destroy. That's useful for testing, and there's a context manager in whoosh.util.testing that does that, but I think it might confuse people, like making it a context manager might imply that that's an expected way to use it. http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:402: return "%s(%s)" % (self.__class__.__name__, repr(self.folder)) On 2012/09/15 14:00:30, ThomasJWaldmann wrote: > "%s(%r)" and no repr()? This is really old code, from before I found out about %r ;) http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:435: if not os.path.exists(dirpath): On 2012/09/15 14:00:30, ThomasJWaldmann wrote: > why not just call makedirs directly without checking first? That would make for cleaner code but just as a personal judgement call I'd like calling .create() a second time to be a no-op instead of an error. I've removed the check and cleaned this up a bit though. http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:440: e = sys.exc_info()[1] On 2012/09/15 14:00:30, ThomasJWaldmann wrote: > why not just: > > except IOError, e: That doesn't work in Python 3. You need to do this stupid trick because the syntax is different.
Sign in to reply to this message.
some clarifications / replies. if you refine this, can you try to update this codereview? one can give -i 6494142 to submit an update to same cr. not sure if another user than the original poster can do it, though. http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py File src/whoosh/filedb/filestore.py (right): http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:291: """ > That doesn't make much sense to me, since an index is meant to be persistent, > not something you create for a few operations and the destroy. I was referring to close() (not destroy()) and usage like this: on a high level in your code: with Storage(...) as st: app.st = st # or pass it by param (call lots of core code here) > That's useful for testing, I think "with" is generally useful if you want to make sure your stuff gets closed no matter what. Of course one is free to not use "with" if one feels better with try/finally, but it often is easier and shorter with "with". As long as nothing bad happens without closing (e.g. because close is a NOP anyway), this is maybe not that obvious, but it gets more obvious if calling close() is really required and things go wrong if you don't. That's my experience since running stuff under pypy (esp. for file objects). http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:435: if not os.path.exists(dirpath): i was just suggesting to drop the exists() test. you can still catch the error from makedirs() and examine it. http://codereview.appspot.com/6494142/diff/1/src/whoosh/filedb/filestore.py#n... src/whoosh/filedb/filestore.py:440: e = sys.exc_info()[1] ah, ok. didn't know that trick as I am not regularly using py3 yet.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||
