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

Rename _CryptoExtras to CryptoExtras #281

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

0xTim
Copy link
Contributor

@0xTim 0xTim commented Oct 16, 2024

Rename _CryptoExtras to CryptoExtras

Checklist

  • I've run tests to see all new and existing tests pass
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

If you've made changes to gyb files

  • I've run .script/generate_boilerplate_files_with_gyb and included updated generated files in a commit of this pull request

Motivation:

CryptoExtras is now depended on by a number of libraries and well used and well understood. This PR formalises this with the removal of the underscore to the product to denote it's now a 'real' library

Modifications:

Renamed the product

Result:

import _CryptoExtras is now import CryptoExtras

@@ -79,7 +79,8 @@ let package = Package(
],
products: [
.library(name: "Crypto", targets: ["Crypto"]),
.library(name: "_CryptoExtras", targets: ["_CryptoExtras"]),
.library(name: "_CryptoExtras", targets: ["CryptoExtras"]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One for discussion - the README is set up that this library won't exist but I've left it here for discussion if we want to keep it to maintain backwards compatibility or not or release this with the upcoming major version

Copy link
Contributor

Choose a reason for hiding this comment

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

I think retaining the backward compatibility is valuable: while the underscore is present, we've tried to avoid gratuitously breaking things and it'd be nice to do so 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.

Ok I've updated the README. In terms of getting this to work with CMake do I need an shim for _CryptoExtras that points to CryptoExtras?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think so.

@0xTim
Copy link
Contributor Author

0xTim commented Oct 16, 2024

One thing this PR doesn't currently change is the CMake files - will wait to hear if it should be a breaking or backwards compatible change first

@Lukasa
Copy link
Contributor

Lukasa commented Oct 18, 2024

I'm going to suggest we slow-roll this until we've had the conversation about the best name for this module.

@fpseverino fpseverino mentioned this pull request Dec 12, 2024
5 tasks
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