-
Notifications
You must be signed in to change notification settings - Fork 110
Conversation
reader = csv.DictReader(file) | ||
for row in reader: | ||
try: | ||
title = unicode(row['Title'], 'utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why title and speaker are being retrieved like this while the other fields below are being retrieved with dict.get()
(which seems more appropriate). Can it not be done that way for all fields?
I'm guessing it's because you're treating title and speaker as required fields and want to raise an exception when either doesn't exist. Why not skip talks that don't have it or use a default value like 'N/A', instead of stopping the import?
|
||
except IOError: | ||
log.error("CSV: File %s not found", file) | ||
if not importer.get_presentations_list(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability, I suggest
presentations = importer.get_presentations_list()
if presentations:
for presentation in presentations:
...
else:
log.info(...)
Refactored plugin.py, added importer plugin category. Refactored database.py to use rss feedparser plugin. Added test for rss importer to test_database.py Related to Freeseer#151, Freeseer#404, Freeseer#413
Refactored plugin.py, added importer plugin category. Refactored database.py to use rss feedparser plugin. Added test for rss importer to test_database.py Related to Freeseer#151, Freeseer#404, Freeseer#413
feedparser = FeedParser(entry) | ||
plugin = self.plugman.get_plugin_by_name("Rss FeedParser", "Importer") | ||
feedparser = plugin.plugin_object | ||
feedparser.pres_list = feedparser.get_presentations_list(entry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? This is still storing the presentation list as state inside the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no particular reason, just the way I thought of it while typing, I'll change it to a local variable.
Refactored plugin.py to add importer plugin category. Refactored database.py to use rss and csv plugins. Added tests for rss and csv importers to test_database.py. Related to issue Freeseer#151, PR Freeseer#413, closes Freeseer#404 PR
Author = Rio Lowry | ||
Version = 3.0.9999 | ||
Website = http://fosslc.org | ||
Description = CSV Importer plugin allows user to importfrom csv files when adding presentations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a space between "importfrom".
Refactored plugin.py to add importer plugin category. Refactored database.py to use rss and csv plugins. Added tests for rss and csv importers to test_database.py. Related to issue Freeseer#151, PR Freeseer#413, closes Freeseer#404 PR
Looks good to me. Thank you very much! |
Merged. Thanks for your contributions! |
per issue #151, adding RSS and CSV importer plugin. Opening this a separate pull request from #404