Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove Application dependency from UrlDispatcher #1510

Merged
merged 2 commits into from
Dec 27, 2016
Merged

Conversation

fafhrd91
Copy link
Member

I think UrlDispatcher should not depend on Application object.
Here is backward compatible change to remove this dependency.

@asvetlov

@fafhrd91
Copy link
Member Author

fafhrd91 commented Dec 25, 2016

we need to update docs and add deprecation warning to router.add_subapp

i think UrlDispatcher should be part of public api

@codecov-io
Copy link

codecov-io commented Dec 25, 2016

Current coverage is 98.69% (diff: 58.06%)

Merging #1510 into master will decrease coverage by 0.17%

@@             master      #1510   diff @@
==========================================
  Files            30         30          
  Lines          6986       7070    +84   
  Methods           0          0          
  Messages          0          0          
  Branches       1163       1180    +17   
==========================================
+ Hits           6907       6978    +71   
- Misses           39         52    +13   
  Partials         40         40          

Powered by Codecov. Last update 6da8ae9...dc13304

raise RuntimeError(
"Cannod add sub application to frozen application")
if subapp.frozen:
raise RuntimeError("Cannod add frozen application")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"cannod" typo


def _wrap_add_subbapp(app):

def add_subapp(prefix, subapp):
if subapp.frozen:
raise RuntimeError("Cannod add frozen application")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...a-ha, copied from here — "cannod"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

@fafhrd91
Copy link
Member Author

@asvetlov objections?

also what about UrlDispatcher as part of public api?

@asvetlov
Copy link
Member

Hmm. I dislike router.add_subapp = _wrap_add_subbapp(self) -- looks like a hack.

Also I doubt if it will work with all possible router implementations.
Adding add_subapp to Application doesn't look reasonable -- we are adding a way to register sub applications only but not regular routes.

I suggest keeping the code as is.

@fafhrd91
Copy link
Member Author

router.add_subapp just for backward compatibility until next release

@fafhrd91 fafhrd91 merged commit 0cd24ab into master Dec 27, 2016
@fafhrd91 fafhrd91 deleted the refactor-subapps branch December 27, 2016 17:14
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants