Bnb/log interp

[bnb32]

No description provided.

[grantbuster]

why base/concrete classes? is BaseEraDownloader sub-classed somewhere else? Are there abstract methods?

[grantbuster]

@bnb32 looks good but still curious why you did BaseEraDownloader and EraDownloader. Seems like you're not using the base class.

Also, in the future would you mind responding to all review comments? Otherwise you make me go through and manually check how you responded to all of them. This should be a dialogue, not just throwing stuff over the fence.

[bnb32]

@bnb32 looks good but still curious why you did BaseEraDownloader and EraDownloader. Seems like you're not using the base class.

Also, in the future would you mind responding to all review comments? Otherwise you make me go through and manually check how you responded to all of them. This should be a dialogue, not just throwing stuff over the fence.

@grantbuster I used the Base class just because it felt cleaner. I put what seemed like more general methods there. Happy to combine them though. My bad about not responding, thought you would get notifications or something for what I did respond to and see code changes otherwise.

[grantbuster]

@bnb32 looks good but still curious why you did BaseEraDownloader and EraDownloader. Seems like you're not using the base class.
Also, in the future would you mind responding to all review comments? Otherwise you make me go through and manually check how you responded to all of them. This should be a dialogue, not just throwing stuff over the fence.

@grantbuster I used the Base class just because it felt cleaner. I put what seemed like more general methods there. Happy to combine them though. My bad about not responding, thought you would get notifications or something for what I did respond to and see code changes otherwise.

Do you see a potential subclass using only the base class and not the full era downloader class? If not, i'd say combine them. Inheritance hierarchies should only be as deep as is strictly necessary. The purpose of abstraction is to provide a generic template for children classes. Without multiple children classes, it's just an unnecessary complication to structure IMO.

Yeah I get like an email when you push to an open PR but it's much nicer to have responses to queries rather than digging through code (again) to figure out what you did and guessing on whether or not i think you addressed the question.

[bnb32]

Do you see a potential subclass using only the base class and not the full era downloader class? If not, i'd say combine them. Inheritance hierarchies should only be as deep as is strictly necessary. The purpose of abstraction is to provide a generic template for children classes. Without multiple children classes, it's just an unnecessary complication to structure IMO.

@grantbuster I could maybe envision something that downloads other data sources but its prob not worth keeping them separate right now. I hear ya on the responses :)

Yeah I get like an email when you push to an open PR but it's much nicer to have responses to queries rather than digging through code (again) to figure out what you did and guessing on whether or not i think you addressed the question.

[grantbuster]

Do you see a potential subclass using only the base class and not the full era downloader class? If not, i'd say combine them. Inheritance hierarchies should only be as deep as is strictly necessary. The purpose of abstraction is to provide a generic template for children classes. Without multiple children classes, it's just an unnecessary complication to structure IMO.

@grantbuster I could maybe envision something that downloads other data sources but its prob not worth keeping them separate right now. I hear ya on the responses :)

Yeah i say let's merge them and then merge this PR! sweet!