[Twisted-Python] Unruly Callback Code

[Justin Johnson]

How's it look now?  I still need to add appropriate errbacks for each
deferred in the deferred list, as opposed to using an errback on the
entire list.  Any other suggestions (even minor) about variable/function
naming or anything else?


	# Subcommand: mkvob
	def cmd_mkvob(self):
		"""Usage: mkvob -v vob-tag[,vob-tag,...] -o site [-g group] [-s site,site,...]
		"""
		options = self.config.subOptions
		original_site = options["original"]
		group = options["group"]
		vobs = options["vobs"].split(",")
		sites = []
		if options["sites"] is not None:
			sites = options["sites"].split(",")

		def renameReplica(results, perspective, vobs, original_site):
			d = perspective.callRemote("renameReplica", vobs, "original", original_site)
			return d

		def mkRepExport(results, perspective, vobs, sites):
			d = perspective.callRemote("mkReplicaExport", vobs, sites)
			return d

		def _stop(fail):
			reactor.stop()
			return fail

		def mkRepImport(results, perspective, vobs, sites):
			if len(vobs) > 0 and len(sites) > 0:
				deferreds = []
				for site in sites:
					def applyTriggers(results, perspective, vobs):
						d = perspective.callRemote("applyTriggers", vobs)
						return d
					def gotObject(perspective, vobs):
						d = perspective.callRemote("mkReplicaImport", vobs)
						d.addCallback(applyTriggers, perspective, vobs)
						return d
					d = self.connectToServer(config.siteToServer[site])
					d.addCallback(gotObject, vobs)
					deferreds.append(d)
				return defer.DeferredList(deferreds)
			return results
				
		def gotObject(perspective, vobs, group, original_site, sites):
			d = perspective.callRemote("mkvob", vobs, group)
			d.addCallback(renameReplica, perspective, vobs, original_site)
			d.addCallback(mkRepExport, perspective, vobs, sites)
			d.addCallback(mkRepImport, perspective, vobs, sites)
			d.addErrback(log.err)
			d.addBoth(_stop)
			return d

		# Connect to the server and start the chain of deferreds
		d = self.connectToServer(config.siteToServer[original_site])
		d.addCallback(gotObject, vobs, group, original_site, sites)
		reactor.run()

On Tue, 5 Aug 2003 19:02:44 +0300, "Tommi Virtanen"
<tv at twistedmatrix.com> said:
> On Tue, Aug 05, 2003 at 08:02:31AM -0600, Justin Johnson wrote:
> > Would someone mind looking at the code below and suggesting ways to clean
> > this up?
> 
> > 	# Subcommand: mkvob
> > 	def cmd_mkvob(self):
> > 		"""Usage: mkvob -v vob-tag[,vob-tag,...] -o site [-g group] [-s site,site,...]
> > 		"""
> > 		options = self.config.subOptions
> > 		original_site = options["original"]
> > 		group = options["group"]
> > 		vobs = options["vobs"].split(",")
> > 		sites = []
> > 		if options["sites"] != None:
>  		if options["sites"] is not None:
> > 			sites = options["sites"].split(",")
> > 
> > 		def gotObject(object):
> 		def gotObject(object, vobs, group):
> > 			self.perspectives[original_site] = object
> > 			print "Successfully connected to %s (%s)." % (original_site, config.siteToServer[original_site])
> > 			print "Calling remote mkvob ..."
> > 			d = object.callRemote("mkvob", vobs, group)
> > 			return d
> 
> 	I'd actually avoid passing vobs and group into gotObject like
> 	that. Just have them as parameters; that means the function
> 	body is independent of its location, which I consider a lot
> 	cleaner.
> 
> > 		def renameReplica(results, vobs, original_site):
> > 			# Do I have to connect again to do this?
> > 			obj = self.perspectives[original_site]
> > 			obj.callRemote("renameReplica", vobs, "original", original_site)
> > 			return obj
> > 
> > 		def mkRepExport(results, vobs, sites, original_site):
> > 			obj = self.perspectives[original_site]
> > 			d = obj.callRemote("mkReplicaExport", vobs, sites)
> > 			return d
> 
> 	The tricks you play with self.perspectives are quite
> 	confusing, IMHO. I'd be more inclined to adding the
> 	callbacks accessing self.perspectives at a stage where
> 	I could pass them the perspective as an argument.
> 	That is,
> 
> 		def gotObject(object, vobs, group):
> 			d = object.callRemote("mkvob", vobs, group)
> 			d.addCallback(renameReplica, object, vobs, original_site)
> 			d.addCallback(mkRepExport, object, vobs, sites, original_site)
> 			d.addCallback(mkRepImport, object, vobs, sites)
> 			return d
> 
> 	Also, object is a bad name, as that name means other things in
> 	python2.2.
> 
> > 		def _stop(*args):
> > 			print "The mkvob operation completed successfully"
> > 			reactor.stop()
> > 			return args
> 
> 		def _stop(fail):
> 			reactor.stop()
> 			return fail
> 
> 	That's not true, _stop is also called on errors.
> 
> 	Also, _stop gets no extra arguments in .addBoth, so args=(aFailure,)
> 	and then you return that tuple.. 
> 
> > 		def mkRepImport(results, vobs, sites):
> > 			if len(vobs) > 0 and len(sites) > 0:
> > 				deferreds = []
> > 				for site in sites:
> > 					def onSuccess(object, site):
> > 						self.perspectives[site] = object
> > 						d = object.callRemote("mkReplicaImport", vobs)
> > 						return d
> > 					def applyTriggers(results, vobs, site):
> > 						obj = self.perspectives[site]
> > 						d = obj.callRemote("applyTriggers", vobs)
> > 						return d
> > 					d = self.connectToServer(config.siteToServer[site])
> > 					d.addCallback(onSuccess, site).addErrback(log.err)
> > 					d.addCallback(applyTriggers, vobs, site).addErrback(log.err)
> > 					deferreds.append(d)
> > 				return defer.DeferredList(deferreds)
> > 			return results
> 
> 	You just stepped in the DeferredList trap. DeferredList
> 	_never_ .errback()s unless it's told to fireOnOneErrback.
> 	That means you miss what errors might have happened.
> 
> 	And fireOnOneErrback isn't normally what you want; you want
> 	to handle the results one by one.
> 
> > 		d = self.connectToServer(config.siteToServer[original_site])
> > 		d.addCallbacks(gotObject, gotNoObject)
> > 		d.addCallback(renameReplica, vobs, original_site).addErrback(log.err)
> > 		d.addCallback(mkRepExport, vobs, sites, original_site).addErrback(log.err)
> > 		d.addCallback(mkRepImport, vobs, sites).addErrback(log.err)
> 
> 	There's no need to .addErrback that much. There's probably no
> 	reason to have gotNoObject special cased, and handle later
> 	errors just with log.err.
> 
> 		d = self.connectToServer(config.siteToServer[original_site])
> 		d.addCallback(gotObject, vobs, group)
> 		d.addCallback(renameReplica, vobs, original_site)
> 		d.addCallback(mkRepExport, vobs, sites, original_site)
> 		d.addCallback(mkRepImport, vobs, sites)
> 		d.addErrback(log.err)
> 
> -- 
> :(){ :|:&};:
> 
> _______________________________________________
> Twisted-Python mailing list
> Twisted-Python at twistedmatrix.com
> http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python