search

underyx / flask-redis

A Flask extension for using Redis
Other
436 stars 71 forks source link

Update from_custom_provider args #41

Closed dayiguizhen closed 4 years ago

dayiguizhen commented 6 years ago

It is little confuse two args have same name app.And in general, this two variables have same value.

codecov-io commented 6 years ago

Codecov Report

Merging #41 into master will decrease coverage by 1.35%. The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #41      +/-   ##
=======================================
- Coverage   51.35%   50%   -1.36%     
=======================================
  Files           1     1              
  Lines          37    36       -1     
=======================================
- Hits           19    18       -1     
  Misses         18    18
Impacted Files Coverage Δ
flask_redis/__init__.py 50% <0%> (-1.36%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 179a4d9...1e8d1a1. Read the comment docs.

underyx commented 4 years ago

Heya! Thanks for the PR. Sorry that I took such an incredibly long time to respond!

The thing is, I think your PR would've introduced a bug. See the comment at the top of the method:

        # We never pass the app parameter here, so we can call init_app
        # ourselves later, after the provider class has been set
        instance = cls(**kwargs)

If you stop extracting app as a keyword parameter, it'd be passed into the class up there right at the start, before there's a chance for us to set a custom provider. Therefore, init_app would be called with the wrong provider.

Does that make sense? I'm gonna close this PR now, but I appreciate the help!