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

Common API Between Flask + Blueprint #3709

Merged
merged 1 commit into from
Aug 1, 2020

Conversation

YKo20010
Copy link
Contributor

Resolves #3215

@YKo20010
Copy link
Contributor Author

YKo20010 commented Jul 30, 2020

Still wondering what to do about the @setupmethod from our previous experimental branch draft PR:
image

@davidism
Copy link
Member

Skeleton, Flask, and Blueprint should use setupmethod consistently. The fact that blueprints weren't using it (or some form of it) is probably an oversight. The point of setupmethod is to ensure those methods aren't called after the application starts because it would lead to inconsitent results. That seems even more true for blueprints, where adding a route, etc. to a blueprint after it's registered means it won't have been added to the app.

@davidism
Copy link
Member

Since setupmethod is specific to Flask right now (uses debug and _got_first_request, which are Flask-only), this could be extracted to a helper method that both Flask and Blueprint implement.

def setupmethod(f):
    @wraps(f)
    def wrapped(self, *args, **kwargs):
        if self._is_setup_finished():
            raise AssertionError(...)

        return f(*args, **kwargs)

    return wrapper

class Skeleton:
    def _is_setup_finished(self):
        raise NotImplementedError

class Flask:
    def _is_setup_finished(self):
        return self.debug and self._got_first_request

class Blueprint:
    def _is_setup_finished(self):
        return self.warn_on_modifications and self._got_registered_once

@davidism
Copy link
Member

This does change the behavior a bit, since Blueprint.warn_on_modifications shows a warning instead of raising an error. For some reason, warn_on_modifications is disabled by default as well, and I'm not really clear why an AssertionError was used instead of a RuntimeError. I'll address that in a later issue though.

@davidism
Copy link
Member

Regarding naming, a few of us in Discord liked the name Scaffold, it fits with Pallets and Blueprint. I'll let it sit a while longer, no need to rename it yet.

@YKo20010 YKo20010 force-pushed the 3215-flask-blueprint branch 4 times, most recently from 2d1c9d5 to 73592bd Compare July 30, 2020 20:52
@chrisnguyn
Copy link
Contributor

I saw the messages in the Discord about switching from skeleton.py and class Skeleton to scaffold.py and class Scaffold respectively, just scrolled up and saw your comment about not needing to change it just yet -- sorry about that! 😅 We can always revert if we decide against it. 👍

src/flask/app.py Outdated Show resolved Hide resolved
src/flask/blueprints.py Outdated Show resolved Hide resolved
src/flask/blueprints.py Outdated Show resolved Hide resolved
@YKo20010 YKo20010 force-pushed the 3215-flask-blueprint branch 2 times, most recently from 6420775 to bd96ab2 Compare July 31, 2020 17:00
@davidism davidism added this to the 2.0.0 milestone Aug 1, 2020
@davidism davidism force-pushed the 3215-flask-blueprint branch from bd96ab2 to 2e2e1ed Compare August 1, 2020 14:43
Co-authored-by: Chris Nguyen <chrisngyn99@gmail.com>
@davidism davidism force-pushed the 3215-flask-blueprint branch from 2e2e1ed to b146a13 Compare August 1, 2020 14:46
@davidism
Copy link
Member

davidism commented Aug 1, 2020

Moved a couple minor things into scaffold, added a changelog.

@davidism davidism merged commit 632f85b into pallets:master Aug 1, 2020
@davidism davidism deleted the 3215-flask-blueprint branch August 1, 2020 14:49
@bekab95 bekab95 mentioned this pull request Aug 2, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Identify the common API between Flask and Blueprint classes
3 participants