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

update README to include example for overriding a default generator claims #172

Closed
wants to merge 1 commit into from

Conversation

the-shank
Copy link

Added an example to the README file for overriding a default generator claim, as discussed in #171

@coveralls
Copy link

coveralls commented Sep 18, 2017

Coverage Status

Coverage remained the same at 84.694% when pulling 0f33a8f on the-shank:master into 4e7d34f on bryanjos:master.

@cs-victor-nascimento
Copy link
Contributor

Hey @the-shank Thanks for your PR!

There is plenty of whitespace diff here and I think that pollutes a bit of the history. When the source code formatter is built-in (see elixir-lang/elixir#6639) we will probably reformat the whole source code.

One peril of your example is that it will validate if 24 * 60 * 60 is less then current_time(). That will always be true (unless you are simulating very old times). So, your expiration should be: current_time() + 24 * 60 * 60 otherwise your token will probably never expire.

I'm planning on rewriting a bit of the API to make all of this simpler and I also plan on adding plenty more documentation (examples, use cases, recommendations and the like).

Could you please address this issues? Thanks once again.

@the-shank the-shank closed this Dec 27, 2017
@the-shank the-shank reopened this Dec 27, 2017
@the-shank
Copy link
Author

@cs-victor-nascimento sorry for not taking care of this for so long. Is this still something that I should look into or have you already gotten around to adding more documentation already?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.694% when pulling 0f33a8f on the-shank:master into 4e7d34f on bryanjos:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.694% when pulling 0f33a8f on the-shank:master into 4e7d34f on bryanjos:master.

@coveralls
Copy link

coveralls commented Dec 27, 2017

Coverage Status

Coverage remained the same at 84.694% when pulling 0f33a8f on the-shank:master into 4e7d34f on bryanjos:master.

@victorolinasc
Copy link
Collaborator

This is still desirable yes. I have plans on overhauling the docs and API during January but this contribution will still be valid for current versions.

@victorolinasc
Copy link
Collaborator

Our current API in 2.0.0-rc0 has a a re-write to make it more explicit. We've also added guides that show how to customize your token configuration.

Thanks for your contribution and we hope this API is clearer. If you could take a stab at it we'd appreciate.

If you still think the docs aren't good enough, please open a new PR on top of current master. Sorry for inconveniences.

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.

4 participants