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

Make jwt::decoded_jwt moveable #225

Merged
merged 1 commit into from
Apr 12, 2022
Merged

Conversation

Ka0o0
Copy link
Contributor

@Ka0o0 Ka0o0 commented Apr 12, 2022

This removes the const specifier from the token member class so that objects of type jwt::decoded_token can be moved. IMO it is safe to do so as the variable is only exposed via get_token which returns a const reference to it.

I'm unsure though in regards to whether or not to make it private. Currently is protected, making it private would then make it a breaking change.

This resolves #224 then.

@Thalhammer
Copy link
Owner

Thalhammer commented Apr 12, 2022

While I am in no way against making it private (you are not really supposed to use it and modifying it wasnt possible anyway) I don't really see a benefit in doing so 🤔

The change itself looks good to merge.

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Apr 12, 2022

Hi Dominik,
thank you for the quick response. Yes true. I was just considering this because the member was const but I also think it's fine to keep it protected as also the other fields are protected. Then I guess this PR is ready for merge.

@Thalhammer Thalhammer merged commit 5c588fc into Thalhammer:master Apr 12, 2022
@Thalhammer
Copy link
Owner

Thanks for finding and fixing this. Great work. We might reconsider making this stuff private for the next major breaking release, but I'd like to avoid breaking changes (regardless how minor they are) unless we have to or there are already breaks anyway.

@Ka0o0 Ka0o0 deleted the moveable-token branch April 19, 2022 11:28
@Ka0o0 Ka0o0 restored the moveable-token branch April 26, 2022 12:44
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.

Make jwt::decoded_jwt moveable
2 participants