Hi Erik. In addition to Norman's feedback...
def descriptor_csv_exp(descr, head=True, incl_fields=[], excl_fields=[]):
It's redundant to call this descriptor_csv_exp() since it's in 'stem.descriptor.*'. Lets just call it something simple like export_csv(). Also, I still think that we should combine the descriptor_csv_exp() and descriptors_csv_exp() in a similar fashion to the reader and controller methods.
doc_loc = os.path.expanduser('~') + '/torexport.csv' doc = open(doc_loc, 'w')
Obviously something to change. :)
We should have a function that returns us a csv string, and maybe another that writes it to a file instead.
# define incl_fields, 4 cases where final case is incl_fields already # defined and excl_fields left blank, so no action is necessary. if not incl_fields and excl_fields: incl_fields = attr.keys() for f in excl_fields: if f in incl_fields: incl_fields.remove(f)
Alternatively you could just default incl_fields and excl_fields earlier...
if not descrs: return ""
# default to including all of the parameters if incl_fields is None: incl_fields = vars(descrs[0]).keys()
if excl_fields is None: excl_fields = []
Also, lets fully spell these out (descriptors, include_fields, exclude_fields) since this is exposed to our user via the keyword parameter.
final = {} for at in attr: if at in incl_fields: final[at] = attr[at] dwriter.writerow(final)
You're relying on each of your vars(desr) calls to provide the same ordering which, while probably safe, isn't something that we should rely on. Lets iterate over incl_fields instead.
Also, you aren't checking that all of the descriptors are of the same type. If you get a list with both server descriptors and extrainfo descriptors then I'm not sure what this function will do, but it's probably not what the user wants. Please include that use case while writing unit tests.
For the future I would suggest opening a ticket on trac and asking for a code review there. This is what Ravi does and it seems to work pretty well - here's an example... https://trac.torproject.org/6114
Cheers! -Damian