search

pgh-public-meetings / city-scrapers-pitt

Pittsburgh City Scrapers: sourcing public meetings in Pittsburgh
https://pgh-public-meetings.github.io/events/
MIT License
19 stars 66 forks source link

WIP: 0078 spider pa utility #87

Closed thatKennedy closed 4 years ago

thatKennedy commented 4 years ago

I think finished what will be necessary for the PA Utility Commission.

Tests have passed and ran all the linting and style-checking tools.

As this is my first spider I'll consider this a WIP and would like to know if I'm missing anything!

ben-nathanson commented 4 years ago

Thanks for this spider, James! The code was easy to read and I appreciate that you wrote some tests. It doesn't appear to be missing anything besides constants for address and name.

We're using the same value for address and name on each event, but those are defined within the method. It would be better to define them at the top of script as constants, just like you did with the default start time. Finally, for default_start_time and other variables, they should be upper case. I don't see a big difference TBH, but the conventional style is to do all uppercase letters separated by underscores. More on that here.

But those are pretty minor style things so I will go ahead and merge this in. You are welcome to make another PR if you feel like refactoring.

Thanks, --Ben

thatKennedy commented 4 years ago

Thanks for the feedback, Ben!

I'll work on a refactor now.