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

Use correct arguments for custom algo example #617

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

aedryan
Copy link
Contributor

@aedryan aedryan commented Sep 19, 2024

Description

Documentation for OpenSSL::HMAC shows that the arguments are digest, key, data and the example has key and data mixed up.

https://docs.ruby-lang.org/en/master/OpenSSL/HMAC.html

Checklist

Before the PR can be merged be sure the following are checked:

  • There are tests for the fix or feature added/changed
  • A description of the changes and a reference to the PR has been added to CHANGELOG.md. More details in the CONTRIBUTING.md

Copy link
Member

@anakinj anakinj left a comment

Choose a reason for hiding this comment

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

I think the test is with the parameters flipped. If you have interest and time it would be great to verify that it works as intended: https://github.com/jwt/ruby-jwt/blob/main/spec/integration/readme_examples_spec.rb#L461

CHANGELOG.md Outdated Show resolved Hide resolved
@anakinj
Copy link
Member

anakinj commented Sep 19, 2024

Thanks @aedryan for noticing and even bigger thanks for taking a stab fixing it. As mentioned, I think the spec for the example in the readme would need the same treatment.

CHANGELOG.md Outdated
@@ -4,12 +4,9 @@

[Full Changelog](https://github.com/jwt/ruby-jwt/compare/v2.9.0...main)

**Features:**
Copy link
Member

Choose a reason for hiding this comment

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

This part should not be removed. Probably some merge issues when syncing with main...

Copy link
Member

Choose a reason for hiding this comment

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

The changelog should only have additions in PRs. So would be great if you could bring back the red parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I think I borked it up real good trying to reset from main and re-add the new version, I'll try again.

Copy link
Member

Choose a reason for hiding this comment

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

:) good luck

Copy link
Member

Choose a reason for hiding this comment

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

You don't need to worry about version numbers or anything. Just adding your entry above the - Your contribution here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's good now.

Copy link
Member

Choose a reason for hiding this comment

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

Now the - Your contribution here was replaced. If you bring back that one we are good to go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it back.

Copy link
Member

Choose a reason for hiding this comment

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

Was just about to merge but there are some conflicts vs what is on main currently.

There was this 2.9.1 release in between that bumped the version in the changelog, think your branch is still based on the previous revision of main. If you would be so kind to do a rebase (and maybe also squashing the commits while at it)

Sorry for the back and forth and thank you for your patience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I squashed and rebased, I think it looks good now.

@aedryan
Copy link
Contributor Author

aedryan commented Sep 24, 2024

I think the test is with the parameters flipped. If you have interest and time it would be great to verify that it works as intended: https://github.com/jwt/ruby-jwt/blob/main/spec/integration/readme_examples_spec.rb#L461

I get several warnings and pending tests when I run bundle exec appraisal rake test but the README spec seems to pass just fine.

@anakinj
Copy link
Member

anakinj commented Sep 24, 2024

I get several warnings and pending tests when I run bundle exec appraisal rake test but the README spec seems to pass just fine.

I think it's fine, I think it's a bit tricky to get running with older versions of the openssl gem. The CI is green so tests are happy.

@anakinj anakinj merged commit a40f8d7 into jwt:main Sep 25, 2024
34 checks passed
@anakinj
Copy link
Member

anakinj commented Sep 25, 2024

Thank you @aedryan for your effort, and apologize for the confusing changelog activities. Some day we will automate it...

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