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

Jsoncpp traits no longer uses deprecated Reader #357

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

kiddigital
Copy link
Contributor

The jsoncpp trait made use of the deprecated Reader.

This PR fixes this. Safer and no more deprecation warnings when compiling.

@kiddigital kiddigital changed the title Jsoncpp traits no longer uses depricated Reader Jsoncpp traits no longer uses deprecated Reader Jul 25, 2024
@prince-chrismc
Copy link
Collaborator

1.4 when we are using 1.9 I don't think it's a breaking change to requie this but it's worth noting.

@kiddigital
Copy link
Contributor Author

1.4 when we are using 1.9 I don't think it's a breaking change to requie this but it's worth noting.

It has been marked as Deprecated since 1.4 😁
So quite likely that the deprecated Reader gets dropped some time soon in an upcoming version of jsoncpp.
And also very likely that people that are using jsoncpp (in combination with jwt-cpp) will use a more recent version of jsoncpp. At least 1.4.

Who doesn't want to get rid of these deprecated messages. And when incorporating something new like jwt-cpp, getting these deprecated messages does not bring joy 😉

@prince-chrismc
Copy link
Collaborator

I see only 1 linter failed, https://github.com/Thalhammer/jwt-cpp/actions/runs/10100944975/job/27938470947?pr=357#step:5:82

If you don't there's instructions to fix it quickly, of not I will do my best to make time to push it myself.

@kiddigital
Copy link
Contributor Author

I see only 1 linter failed, ...

I saw that as well, but it is the 'process linter results' step 😁

Not something I think I can/should fix. Or did I overlook something?

@prince-chrismc
Copy link
Collaborator

It's telling you there are changes to be applied to the PR 😜 again it tells you the exact command to run to fix it sonics just a copy paste... the code diff is there too if you expand it.

@kiddigital
Copy link
Contributor Author

Ah.. ok.. I need to read better 😜 Will have a look asap and apply the fixes...

Copy link
Owner

@Thalhammer Thalhammer left a comment

Choose a reason for hiding this comment

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

Seems good to me 👍

@kiddigital
Copy link
Contributor Author

@prince-chrismc , you just beat me to it 😁 thx

@prince-chrismc prince-chrismc merged commit b36e8a9 into Thalhammer:master Jul 29, 2024
59 checks passed
@prince-chrismc
Copy link
Collaborator

My pleasure :) Thanks for the contribution! 🚀

@kiddigital kiddigital deleted the patch-1 branch July 29, 2024 05:08
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.

3 participants