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

(MINOR) Move crate-level functions for creating signers to new public create_signer mod #72

Merged
merged 5 commits into from
Jul 14, 2022

Conversation

scouten-adobe
Copy link
Collaborator

@scouten-adobe scouten-adobe commented Jul 12, 2022

Changes in this pull request

Turns out it is legal in Rust to publicly export a function as a top-level entity in a crate, but such a function does not appear in the documentation for the crate.

For that reason, we're moving the get_signer and get_signer_from_files functions into a new create_signer module.

(Update: This turned out to be an incorrect assumption on my part, but we've decided to make this change anyway because the docs are easier to comprehend in the new format.)

Checklist

  • This PR represents a single feature, fix, or change.
  • All applicable changes have been documented.
  • Any TO DO items (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

Codecov Report

Merging #72 (b905488) into main (df9f915) will decrease coverage by 0.01%.
The diff coverage is 26.78%.

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
- Coverage   73.43%   73.42%   -0.02%     
==========================================
  Files          64       64              
  Lines       13279    13279              
==========================================
- Hits         9752     9750       -2     
- Misses       3527     3529       +2     
Impacted Files Coverage Δ
sdk/src/lib.rs 100.00% <ø> (ø)
sdk/src/manifest.rs 88.20% <ø> (ø)
sdk/src/openssl/mod.rs 77.77% <ø> (ø)
sdk/src/utils/test.rs 89.78% <0.00%> (ø)
sdk/src/create_signer.rs 25.92% <25.92%> (ø)
make_test_images/src/make_test_images.rs 73.36% <100.00%> (ø)
sdk/src/openssl/ec_signer.rs 84.86% <0.00%> (-1.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df9f915...b905488. Read the comment docs.

@gpeacock
Copy link
Collaborator

We are currently not documenting anything behind a feature, because that is the default used when publishing via cargo docs. To fix that we need to add this to the c2pa.rs Cargo.toml file:

[package.metadata.docs.rs]
all-features = true

If we make this change, do we still need this PR?

@scouten-adobe
Copy link
Collaborator Author

scouten-adobe commented Jul 13, 2022

The missing-doc issue is mostly addressed via #73, but I would like to discuss this PR as a possibly-preferred API pattern. (A preliminary local test suggests rustdoc doesn't handle this case well, even with the new flags introduced in #73.)

Copy link
Collaborator

@gpeacock gpeacock left a comment

Choose a reason for hiding this comment

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

I'm ok with this. I would be cleaner if Signer was a struct and these were constructor methods on it, but since Signer is a trait, this makes sense.

sdk/src/create_signer.rs Show resolved Hide resolved
@scouten-adobe
Copy link
Collaborator Author

@gpeacock agreed that this would be cleaner if we could make these static methods on Signer itself, but I've re-checked and did not find a way to do that. Merging as is.

@scouten-adobe scouten-adobe merged commit c59dc83 into main Jul 14, 2022
@scouten-adobe scouten-adobe deleted the create-signer branch July 14, 2022 23:21
mauricefisher64 added a commit that referenced this pull request Jul 15, 2022
…ync_sign

* 'main' of https://github.com/contentauth/c2pa-rs:
  Adds an add_validation_status method to Ingredient (#68)
  Prepare 0.8.1 release
  (IGNORE) Disallow overlapping publish runs (#78)
  Use rsa crate for RSA-PSS verification in Wasm (#77)
  Prepare 0.8.0 release
  Add a new API to provide access to COSE signing logic for external signers (#75)
  (MINOR) Move crate-level functions for creating signers to new public `create_signer` mod (#72)
  Prepare 0.7.2 release
  Fix broken documentation build (#74)
  Prepare 0.7.1 release
  Configure docs.rs to include feature-gated items (#73)
  Update XMP Toolkit to 0.5.0 (#71)

# Conflicts:
#	sdk/Cargo.toml
#	sdk/src/cose_sign.rs
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.

None yet

3 participants