Excerpts from Yuya Nishihara's message of 2017-02-05 13:59:32 +0900: > Perhaps that is a matter of taste, I like the explicit one. But I heard > Python 3 has a generator-based coroutine, so using "yield" for that purpose > is more common in Python 3? > > @preload('index') > def preloadindex(repo): > f = repo.svfs('changelog.i') # "f" can be shared naturally > hash = fstat(f).st_size > def loadindex(): > indexdata = f.read() > hash = len(indexdata) > return parseindex(indexdata), hash
The inner "loadindex" function makes things unnecessarily more complex in my opinion. It makes the final return type harder to read, introduces extra indentation and a function definition. If the goal is to get rid of "yield", it could be done in different ways. For example, Choice A: explicitly pass oldhash and the oldobject. We probably need the old object anyway to allow incremental updates: @preload('obsstore', depend=['changelog']) def preloadobsstore(repo, oldhash, oldobsstore): obsstore = oldobsstore newhash = ... if newhash != oldhash: # note: this is an indentation not existed using yield obsstore = ... return newhash, obsstore Choice B: just give the preload function access to the cache object: @preload(['obsstore', 'changelog']) def preloadmultiple(repo, cache): oldhash, oldobsstore = cache['obsstore'] .... Choice A looks simple. Choice B is more expressive while the function body could be out of control - ex. it could skip setting cache['obsstore'] even if it says so in its decorator. While Choice A may serve most requirements just well, I think "yield" is more expressive, and avoid unnecessary indentation. For example, the following could not be expressed using "return" cleanly and easily: - Optional dependency: if fastpath_cannot_work: yield preload.require('changelog') Maybe "raise MissingRequirement('changelog')", but that makes the function "restarted", while the generator could continue executing without losing context. - Optionally change the cache key: As mentioned in "4)", [1] Therefore I think the generator way avoids all kinds of indentations (reminds me of nodejs callback hell), and has better extensibility. So I currently prefer it. Otherwise the "Choice A" may be good enough for now. The code base has already used "yield" for its future implementation used for batched remote peer calls. [1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-February/092584.html > return hash, loadindex > > BTW, when will 'f' be closed if we take the generator-based abstraction? > > def preloadindex(repo): > f = repo.svfs('changelog.i') > try: > yield ... > yield ... > finally: > f.close() _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel