Skip to content

Implement leeway for date validation #83

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

Merged

Conversation

JanBrinker
Copy link
Contributor

@kylef I implemented the leeway as discussed by optional function parameters. This way there are no code breakages.

I also added unit tests for each date validation (both cases: fail without leeway and success with leeway) and added two small integration tests to check whether the leeway is passed through all calls down to the validators.

@JanBrinker
Copy link
Contributor Author

@kylef do you have any clue why XCTestExpectation can't be initialized on Linux the way it is initialized on macOS? That's currently causing the build fails and I can't debug on Linux..

@kylef
Copy link
Owner

kylef commented Sep 8, 2017

I do not think XCTestExpectation is available on Linux, this is because the test running is a lot different. I do not believe there is a run loop created and thus the type of behaviour of XCTestExpectation doesn't make sense.

XCTestExpectation is for asynchronous testing and these tests are not asynchronous, you should be able to remove the use of XCTestExpectation simply.

@JanBrinker
Copy link
Contributor Author

Any clue why Travis is complaining? It says it can't find the build logs for macOS..

@JanBrinker
Copy link
Contributor Author

@kylef checks are through. If you agree with my implementation it would be great to have the PR accepted 👍

@kylef
Copy link
Owner

kylef commented Sep 11, 2017

Any clue why Travis is complaining? It says it can't find the build logs for macOS.
Hit rebuild and seems to work, I think it was Travis flakiness.

I think this is good, I'll give it a review soon and make a release incorporating these changes.

Can you add an entry in CHANGELOG.md describing your changes please?

@kylef
Copy link
Owner

kylef commented Sep 11, 2017

Might be good to add a note about this in the README.md so other people coming into JSONWebToken are aware of the leeway argument.

kylef
kylef previously approved these changes Sep 11, 2017
@JanBrinker
Copy link
Contributor Author

  • Add change to CHANGELOG.md
  • Add documentation to README.md

@kylef kylef merged commit a891e63 into kylef:master Sep 11, 2017
@kylef
Copy link
Owner

kylef commented Sep 11, 2017

Awesome, nice work here @JanBrinker. I appreciate the pull request.

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