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

Fix #163: Some fixes in documentation and add auth example #166

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix #163: Some fixes in documentation and add auth example #166

wants to merge 1 commit into from

Conversation

amoyiki
Copy link

@amoyiki amoyiki commented Dec 22, 2017

this is my first pr in github:)

@dkellner
Copy link
Collaborator

dkellner commented Jan 2, 2018

Thank you for you PR! I've been quite busy the last days but I will review this until sunday.

Copy link
Collaborator

@dkellner dkellner 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 really sorry I did not get back to you as promised. If that's an excuse, I moved to another continent and could not find the spare time before that, even though I thought I would!

I really appreciate having a working auth example. It's something a lot of users want to do and your work will help them tremendously.

I'd just want to fine-tune a few things before this will be merged in master:

  • requirements.txt. So far the other examples don't have their own requirements. As far as I can see, you only really need it for 'itsdangerous'. Do you think renaming the file to additional-requirements.txt and just adding those is sufficient? People then would have to install the top-level requirements.txt in addition to additional-requirements.txt.

  • We should add an example how to "use" this example in the docs. E.g. add curl commands to show how to get 401 Unauthorized first, then how login to get the token and send a subsequent, successful request.

  • Maybe even "refactor" the docs to not have a lot of duplicated code, but use literalinclude to include relevant parts of the example code. See http://www.sphinx-doc.org/en/stable/markup/code.html#directive-literalinclude .

@@ -69,7 +69,7 @@ First we need to create eve-side authentication:
register_views(app)
app.run()

Next step is the `User` SQLAlchemy model:
Next step is the `User` And `Role` SQLAlchemy model:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: "And" -> "and"

from models import Base, Role, User
from views import register_views

pymysql.install_as_MySQLdb()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to use pymysql here? The other examples use a in-memory SQLite database. Would that be sufficient here, too?

@amoyiki amoyiki closed this Feb 17, 2018
@dkellner
Copy link
Collaborator

@amoyiki Did you close this by mistake? If you are not interested in adapting your code, let me know. I will do so and still merge it.

@amoyiki
Copy link
Author

amoyiki commented Feb 22, 2018

@dkellner i want to fix this mistake in this week

@dkellner dkellner reopened this May 13, 2018
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.

2 participants