Skip to content
This repository has been archived by the owner on May 28, 2022. It is now read-only.

151 make RSS/CSV importer plugins #413

Closed
wants to merge 1 commit into from

Conversation

riolowry
Copy link
Contributor

per issue #151, adding RSS and CSV importer plugin. Opening this a separate pull request from #404

reader = csv.DictReader(file)
for row in reader:
try:
title = unicode(row['Title'], 'utf-8')
Copy link
Member

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?

@riolowry
Copy link
Contributor Author

riolowry commented Dec 6, 2013

I've implemented code review changes, rebased to master and into a single commit. The tests all pass and I think that this pull request is ready to be merged to master, @zxiiro and @dideler please let me know if you feel it is ready or if there are further changes that you would recommend. Thanks.


except IOError:
log.error("CSV: File %s not found", file)
if not importer.get_presentations_list():
Copy link
Member

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(...)

riolowry added a commit to riolowry/freeseer that referenced this pull request Dec 7, 2013
riolowry added a commit to riolowry/freeseer that referenced this pull request Dec 7, 2013
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
riolowry added a commit to riolowry/freeseer that referenced this pull request Dec 7, 2013
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)
Copy link
Member

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.

Copy link
Contributor Author

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.

riolowry added a commit to riolowry/freeseer that referenced this pull request Dec 11, 2013
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
@riolowry
Copy link
Contributor Author

@dideler @zxiiro @mtomwing thanks for all of the feedback, I've implemented all of the changes (hopefully didn't miss anything) and rebased into a single commit, would love a final look over to see if it's ready to include in master

Author = Rio Lowry
Version = 3.0.9999
Website = http://fosslc.org
Description = CSV Importer plugin allows user to importfrom csv files when adding presentations
Copy link
Member

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
@dideler
Copy link
Member

dideler commented Dec 11, 2013

Looks good to me. Thank you very much!

@zxiiro zxiiro closed this in 179b666 Dec 11, 2013
@zxiiro
Copy link
Member

zxiiro commented Dec 11, 2013

Merged. Thanks for your contributions!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants