Skip to content

Solved inconsistency in decoders inheritance hierarchy - #1161

Closed
cdeler wants to merge 1 commit into
encode:masterfrom
cdeler:fix_decoders_hierarchy
Closed

Solved inconsistency in decoders inheritance hierarchy#1161
cdeler wants to merge 1 commit into
encode:masterfrom
cdeler:fix_decoders_hierarchy

Conversation

@cdeler

@cdelercdeler commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

@tomchristie,

I tried to find a common ancestor in Decoders hierarchy (just wanted to make it clearer). Unfortunately there is a big gap between e.g. TextDecoder and BrotliDecoder (approach with Callable looks weak, but once Guido advised it python/typing#241 (comment))

If I did something strange or unnecessary, feel free to decline the PR

Beware: I introduced abc here.

@cdeler
cdelerforce-pushed the fix_decoders_hierarchy branch from 22e4e15 to 7ff4bd5CompareAugust 10, 2020 20:43
@lovelydinosaur

lovelydinosaur commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

Okay, so I'm going to close this one off, since it seems like a technical bit of refactoring, in search of a problem that doesn't exist. Or to put it another way - I can't see what you're actually trying to solve here?

Please don't let that put you off tho' - the contributions are appreciated either way!

But let's not introduce extra hierarchy, ABCs, and other trickery unless there's a really clear end-user facing reason why we're doing so. 😀

@cdeler

Copy link
Copy Markdown
ContributorAuthor

@tomchristie , I do agree that it might look like a refactoring without a target, but I wanted to solve this problem:

classDeflateDecoder(Decoder):
# ...def__init__(self, request: "Request") ->None:
self.request=request# <-- here we do not call super type constructor, but just set the request propertyself.first_attempt=True

(it looks the same for GZipDecoder, BrotliDecoder, DeflateDecoder and MultiDecoder inhereted from Decoder with defined __init__ method). Having tried to solve that (adding super() call) I found that the decoders inheritance is strange and tried to fix it

Nevertheless, I accept the idea, that refactoring without a clear target is a root of evil (and just wanted to explain the cause of my PR)

@florimondmanca

Copy link
Copy Markdown
Contributor

Yes, this rings a bell… :) #1095 (comment)

@cdeler
cdeler deleted the fix_decoders_hierarchy branch September 9, 2020 14:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@cdeler@lovelydinosaur@florimondmanca