Brad Hall wrote: > On Thu, Jul 24, 2008 at 10:02:32AM -0700, Brock Pytlik wrote: > >> I'd especially appreciate comments on the changes in >> server/query_engine.py and the changes in manifest.py which appeared in >> v1. (Sorry, I don't have a diff for v1 -> v2, but I'll make sure I can >> generate that from now on.) >> >> http://cr.opensolaris.org/~bpytlik/ips-search-v2/ >> > > So I just have a style suggestion at this point. I think it may be clearer to > abstract some of the functionality in server/query_engine.py. Makes it easier > to see what's going on (to me at least). > > For example, if you abstract the opening, reading, and closing dicts pieces > into functions like: > def open_dicts(raise_on_none=False): > if ss.consistent_open(self._data_dict.values(), > self._dir_path, FILE_OPEN_TIMEOUT_SECS) == None: > if raise_on_none: > raise > search_errors.NoIndexException(self._dir_path) > return False > return True > def read_dicts(): > for d in self._data_dict.values(): > if d == self._data_main_dict: > continue > d.read_dict_file() > def close_dicts(): > for d in self._data_dict.values(): > d.close_file_handle() > > Then the search function (currently this): > def search(self, query): > """ Searches the indexes in dir_path for any matches of query > and returns the results. > """ > > self.dict_lock.acquire() > try: > if ss.consistent_open(self._data_dict.values(), > self._dir_path, FILE_OPEN_TIMEOUT_SECS) == None: > raise > search_errors.NoIndexException(self._dir_path) > try: > for d in self._data_dict.values(): > if d == self._data_main_dict: > continue > d.read_dict_file() > matched_ids, res_ids = > self.search_internal(query) > finally: > for d in self._data_dict.values(): > d.close_file_handle() > return self.get_results(res_ids) > finally: > self.dict_lock.release() > > Becomes: > def search(self, query): > """ Searches the indexes in dir_path for any matches of query > and returns the results. > """ > > self.dict_lock.acquire() > try: > self.open_dicts(True) > try: > self.read_dicts() > (_, res_ids) = self.search_internal(query) > finally: > self.close_dicts() > return self.get_results(res_ids) > finally: > self.dict_lock.release() > > Similarly, the piece in __init__ becomes: > try: > if self.open_dicts(): > try: > self.read_dicts() > finally: > self.close_dicts() > finally: > self.dict_lock.release() > > Anyhow, I'll post more comments as I make more progress into the review (sorry > for taking so long). > > Good idea. And thanks for making me remember about _, I hadn't tried that in python yet. I'll make these changes and wait to hear what else needs fixing before I push another webrev.
Brock > Thanks, > Brad > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
