Skip to content

Conversation

howiezhao
Copy link

This PR adds an example of integrating Flask-Basicauth in the Introduction section of the documentation.

Copy link
Contributor

@samuelhwilliams samuelhwilliams left a comment

Choose a reason for hiding this comment

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

Could we perhaps do this as a working example in the examples/ directory and point to that? This snippet implies a few things already exist (app, Post, db) - and it'd be nice to just be able to see a functioning example straight away.

@howiezhao
Copy link
Author

Could we perhaps do this as a working example in the examples/ directory and point to that? This snippet implies a few things already exist (app, Post, db) - and it'd be nice to just be able to see a functioning example straight away.

Hi @samuelhwilliams Thank you for your reply. The reason why I did not put it in the examples/ is that I think this example is very simple. Its core code is only the following lines:

class MicroBlogAdminIndexView(AdminIndexView):
    @expose('/')
    @basic_auth.required
    def index(self):
        return super().index()

class MicroBlogModelView(ModelView):
    def is_accessible(self):
        return basic_auth.authenticate()

    def inaccessible_callback(self, name, **kwargs):
        return basic_auth.challenge()

This example is very concise compared to the Flask-Login and Flask-Security examples.

As for the app, Post, db, etc. you mentioned, they have actually been mentioned in the context of this document. So, if the user reads this document completely, I don’t think there will be any gaps, what do you think?

@samuelhwilliams
Copy link
Contributor

I think it's alright to rely on context further up in the readme, but that a snippet of code in the README that worked if grabbed just by itself would generally be a better user experience.

I'll accept this PR in a couple days if you don't want to change it, but if you want to update the code snippet to work in isolation that would be great too 🙏

Sign up for free to 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.

2 participants