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

Reorganize files #2880

Merged
merged 8 commits into from
Nov 13, 2024
Merged

Reorganize files #2880

merged 8 commits into from
Nov 13, 2024

Conversation

deuszx
Copy link
Contributor

@deuszx deuszx commented Nov 13, 2024

Motivation

certificate.rs started to grow quite large, and data_types.rs was already huge (almost 1500 lines of code).

Proposal

This PR moves code around and puts them into a common certificate module. Things that were done:

  • Split certificate.rs into a module certificate with separate files for different certificate types (containing respective structs, impls and conversions)
  • Create certificate/hashed.rs and put Hashed struct there (with all of the accompanying code)
  • Move the "old" Certificate type (which is now an alias for GenericCertificate<ConfirmedValue>) to the certificate/mod.rs (with a plan to remove it entirely when the time comes)
  • Move CertificateValue and HashedCertificateValue to certificate/value.rs file.
  • Move LiteCertificate to certificate/lite.rs file.

Test Plan

None, this is just moving code around.

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Links

@deuszx deuszx marked this pull request as ready for review November 13, 2024 16:12
@deuszx deuszx requested review from jvff, ma2bd, Twey and afck November 13, 2024 16:12
signatures,
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we start thinking about having unit tests in the same source files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, do you want me to write them or move around? b/c this PR is just about moving around. I can create issues to write these unit tests.

As for the conversions above - I'm assuming that the e2e tests would catch any errors in the mappings and in the future it (the mapping) will go away entirely as we'll get rid of the Certificate.

Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

Looks good to me!
(Although I'm not looking forward to the merge conflict with #2881. 😩)

@deuszx deuszx merged commit 484471c into main Nov 13, 2024
23 checks passed
@deuszx deuszx deleted the reorganize-files branch November 13, 2024 17:11
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