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

Move add_url function definition out of Dash.__init__() #377

Merged
merged 11 commits into from Oct 8, 2018

Conversation

hinnefe2
Copy link
Contributor

@hinnefe2 hinnefe2 commented Sep 8, 2018

First of all, thanks for a great tool!

This PR moves the add_url utility function out of the __init__ method of the Dash class. This change makes it easier to subclass Dash.

My use case is adding authentication via flask_login. Pulling add_url out lets me subclass like so:

  class MyDash(Dash):                                                                                                                                                     
                                                               
      def add_url(self, name, view_func, methods=('GET',)):                                                                                                               
          self.server.add_url_rule(                                                                                                                                       
              name,                                                                                                                                                       
              view_func=login_required(view_func),                                                                                                                        
              endpoint=name,                                                                                                                                              
              methods=list(methods)                                                                                                                                       
          )  

Note that each view_func is wrapped by login_required so all my dash pages now require authentication.

I think this addresses some of the comments in #214.

Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

I'm ok with theses changes once the method is private.

dash/dash.py Outdated
@@ -217,6 +209,14 @@ def add_url(name, view_func, methods=('GET',)):
self._cached_layout = None
self.routes = []

def add_url(self, name, view_func, methods=('GET',)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Make that "private" by prefixing with _.

@hinnefe2
Copy link
Contributor Author

@T4rk1n Sorry for the delay, I think this is ready for another look. Thanks!

@T4rk1n
Copy link
Contributor

T4rk1n commented Sep 20, 2018

@hinnefe2 I think your merge with master went bad, there is a line that should be in __init__ but is in the add_url method. Move back line 225-226 in __init__.

@T4rk1n
Copy link
Contributor

T4rk1n commented Sep 24, 2018

Thanks, looks good now. 💃

Since this is for wrapping the routes in flask_login, you could add in the add_url method a way to save the route names. You can append the url name to self.routes, it is not used presently. Just make sure it is instanciated before the _add_url calls since it is at line 215. I would use that to solve plotly/dash-auth#59 if you do that.

Then instead of subclassing Dash, you can wrap the routes externally:

for view_name, view_method in (
        (x, app.server.view_functions[x])
        for x in app.routes
        if x in app.server.view_functions
    ):
    app.server.view_functions[view_name] = login_required(view_method)

@rmarren1 can you have a look too ?

@rmarren1
Copy link
Contributor

I like this change. I don't see a drawback to defining add_url as a method of Dash rather than an internal function of __init__, and should give advanced users more flexibility.

Not sure why routes doesn't do anything right now, I think we should populate that in _add_url then we can merge this
💃

@hinnefe2
Copy link
Contributor Author

hinnefe2 commented Oct 2, 2018

@T4rk1n Ok, I think this is ready for another look. Thanks again for your patience!

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 2, 2018

@hinnefe2 Great.

We got a new CONTRIBUTING guide, make sure to you have everything on the pre-merge checklist (update version.py and changelogs) to accelerate the merge/release process.

@hinnefe2
Copy link
Contributor Author

hinnefe2 commented Oct 6, 2018

Alright, changelog is updated and version is bumped, anything else for me to do?

@rmarren1
Copy link
Contributor

rmarren1 commented Oct 8, 2018

Looks great, thanks! 🎊

@rmarren1 rmarren1 merged commit 71fb742 into plotly:master Oct 8, 2018
@rmarren1
Copy link
Contributor

rmarren1 commented Oct 8, 2018

Released dash==0.28.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants