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

ssl: Fix typespec for signature_algs/2 and export signature_algs/0 type #7841

Merged
merged 1 commit into from Nov 10, 2023

Conversation

ahovgaard
Copy link
Contributor

@ahovgaard ahovgaard commented Nov 8, 2023

The type signature_algs/0 is already a list, so the return type for signature_algs/2 should just be signature_algs/0.

Additionally, export the type since it's used in a spec of an exported function and it's already documented.

Copy link
Contributor

github-actions bot commented Nov 8, 2023

CT Test Results

No tests were run for this PR. This is either because the build failed, or the PR is based on a branch without GH actions tests configured.

Results for commit 5d4691c

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@ahovgaard ahovgaard marked this pull request as ready for review November 8, 2023 14:39
@@ -1108,7 +1109,7 @@ append_cipher_suites(Filters, Suites) ->
(Suites -- Deferred) ++ Deferred.

%%--------------------------------------------------------------------
-spec signature_algs(Description, Version) -> [signature_algs()] when
-spec signature_algs(Description, Version) -> signature_algs() when
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a related note: This function may also return {error, term()}. It seems a little unfortunate to make the spec signature_algs() | {error, reason()}, since that doesn't exactly align with the common style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Humm ... I think the error cases was added later, and I think the correct way for this function will be to change the error cases to erlang:error(Reason) so that you get a better error reason than function clause. Please add that to the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense. I've updated the PR.

@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Nov 9, 2023
@garazdawi
Copy link
Contributor

The github actions failure are due to errors in the github scripts that should now be fixed. Please rebase your branch to latest maint and they should be solved. Thanks.

@IngelaAndin IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Nov 9, 2023
@IngelaAndin IngelaAndin merged commit f74018f into erlang:maint Nov 10, 2023
15 checks passed
@IngelaAndin
Copy link
Contributor

Thank you for the PR

@ahovgaard ahovgaard deleted the ssl/fix-signature-algs-spec branch November 10, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants