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

Small farmer refactoring #1782

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Small farmer refactoring #1782

merged 2 commits into from
Aug 9, 2023

Conversation

nazar-pc
Copy link
Member

@nazar-pc nazar-pc commented Aug 9, 2023

First commit fixes annoying thing introduced in #1778 where specified farm path must exist before use, now it will be created automatically, but only one level deep to prevent surprising behavior.

Second commit is changes extracted from checksumming branch that tries to improve readability, remove some boilerplate and hide some internals of subspace-farmer-components that do not need to be exposed publicly.

Ignore whitespace changes, then the diff is quite small.

Code contributor checklist:

@@ -92,25 +92,31 @@ impl SectorMetadata {

/// Commitment and witness corresponding to the same record
#[derive(Debug, Default, Clone, Encode, Decode)]
pub struct RecordMetadata {
pub(crate) struct RecordMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

Don't you think pub(crate) for the struct only is enough? Is it somewhere in the rust guidelines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fields and methods were also made private, what do you mean by struct only?

I was just hiding data structures that are internal to subspace-farmer-components and not exposed in public API. This helps ensuring that when in later PR I'm adding checksums, all users of these data structures are doing checksums the same way consistently.

Copy link
Member

Choose a reason for hiding this comment

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

I meant that by making the struct private you prevent leaking the type outside of the crate and all methods and fields are unaccessible respectively. pub methods and fields are less noisy than pub(crate). But maybe it's considered malpractice by the Rust guideline with a good reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean if you need to make it visible publicly, you use pub, if only in parent module you use pub(super) and if only inside of the crate then pub(crate). Using pub when it is not exposed publicly is misleading and also results in data structures not reported as unused after refactoring. So everything that can be private should be private and as private as possible.

@nazar-pc nazar-pc merged commit 5dcb2ad into main Aug 9, 2023
9 checks passed
@nazar-pc nazar-pc deleted the small-farmer-refactoring branch August 9, 2023 07:01
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