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

Making JwtEncoder.Validate() public #128

Merged
merged 6 commits into from
Jul 24, 2017
Merged

Making JwtEncoder.Validate() public #128

merged 6 commits into from
Jul 24, 2017

Conversation

abatishchev
Copy link
Member

Resolves #126

@abatishchev abatishchev self-assigned this Jul 18, 2017
@abatishchev abatishchev added this to the 3.0 milestone Jul 18, 2017
@abatishchev
Copy link
Member Author

@nskhara please validate whether it'd work for you

@abatishchev abatishchev requested a review from nbarbettini July 18, 2017 15:35
Copy link
Contributor

@nbarbettini nbarbettini left a comment

Choose a reason for hiding this comment

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

No issues, but it seems like an odd method to make public. As discussed in the issue, the IJwtValidator is a better interface for this (that already exists), and the signature of this method is not really nice for consumption (it's just an internal method). Seems like it'd be cleaner to just use the IJwtValidator interface.

@abatishchev
Copy link
Member Author

My doubt is IValidator becomes very coupled with IDecoder. But I think it's fine. Shall we rework the interface (make public method private and private method public)?

@nbarbettini
Copy link
Contributor

I'm not quite following what you mean - but I'm happy to review a PR. 😃

@nskhara
Copy link

nskhara commented Jul 19, 2017

@abatishchev
Thank you for the change.

One thing, would it be nice if you could add the method into the IJwtDecoder interface also?
(So you can access it through the interface)

IJwtDecoder decoder = new JwtDecoder(blah, blah, ...);
// some other processess...
decoder.Validate(blah, blah,...);

@abatishchev
Copy link
Member Author

Yes, that's what we've discussed above and I'll address once AppVeyor is functional again. Later this week for sure since I already started.

@abatishchev
Copy link
Member Author

Unfortunately I didn't find this feasible to move this Validate() method to Validator. I refactored it thought to simplify the signature. Please try yourself or/and let me know if it's fine to merge it as it is for now

@abatishchev
Copy link
Member Author

Shall I check this in?

@abatishchev abatishchev merged commit 8432419 into master Jul 24, 2017
@abatishchev abatishchev deleted the encoder-validate branch July 24, 2017 18:24
@nskhara
Copy link

nskhara commented Jul 25, 2017

thank you, abatishchev :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants