Hi Damian,
Here is a first draft for the tor export module we have been discussing. I have attached the file as we were having some git issues and I am in a bit of a rush right now. We have been developing the file at stem/descriptors/export.py. Any suggestions or comments would be appreciated -- we have done basic testing, and would be happy to implement some unit & integration tests if this implementation satisfies the needs of the community.
Thanks, Erik and Megan.
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
On 7/12/12 12:15 PM, Damian Johnson wrote:
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.
Isn't this a bizarre use-case? I.e., what is a use-case in which a client wants a csv of a ((ServerDescriptor + ExtraInfoDescriptor) list)? I.e., a list of a disjoint union type?
- Norman
Maybe you could comment on the comments, or at least keep an eye on it
Replied. I clicked watch but I'm not familiar with how github code reviews work so I might not notice updates.
Also, I don't suppose you know how to comment on the file, instead of on the diff?
I don't. Personally I do code reviews by pulling their changes and diffing with where they branched off master. Github's new to me.
Isn't this a bizarre use-case? I.e., what is a use-case in which a client wants a csv of a ((ServerDescriptor + ExtraInfoDescriptor) list)? I.e., a list of a disjoint union type?
Agreed that it's weird which is why we need a test. I'm fine with having the export function throw a ValueError if it gets mixed descriptor types.
This issue is something that users of the descriptor reader are likely to come across. For instance if you point the reader at tor's data directory then it'll crawl over the cached-descriptors, cached-extrainfo-descriptors, and cached-consensus, giving you a generator for each of their contents.
Cheers! -Damian
-
Damian Johnson -
Erik I Islo -
Norman Danner