search

openstate / open-cultuur-data

The back- and front-end code that powers the Open Cultuur Data API
http://opencultuurdata.nl/
28 stars 18 forks source link

A2a tilburg #76

Closed breyten closed 9 years ago

breyten commented 9 years ago

Implementation for Regionaal Archief Tilburg. Since this is also a genealogical dataset, I've factored out common code and put it into A2AItem. The refactoring contains fixes for major crawling bugs and has test cases and documentation associated with it.

bartdegoede commented 9 years ago

I started with commenting inline, but there are many PEP8 violations, as well as Dutch language within the code. Please fix these first.

breyten commented 9 years ago

Hmm, my famed pep8 extension does not give errors anymore ;-) Can you give an example?

bartdegoede commented 9 years ago

Thanks for the changes, looks much better :-)

I'm almost done complaining: personally, I don't like the style of newlines as used in _get_description. I would not write it as:

def _get_description(
        self, institutionName, sourceType, sourcePlace, allPersons
    ):

but rather as:

def _get_description(self, institutionName, sourceType, sourcePlace, 
                     allPersons):

Also, PEP8 states that variables i functions should not be camelcased :-) Finally, I found some variables that are not used. Please refer to my inline comments for those. When those are fixed, I'm good to go for merge.

breyten commented 9 years ago

I'm meeting your half way by removing dangling ) and ]'s. That's ok?

bartdegoede commented 9 years ago

Yep. Merged and closed.