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

Difficult to read roles::test-utils::mining-device in single file #1174

Open
4 tasks
rrybarczyk opened this issue Sep 30, 2024 · 3 comments · May be fixed by #1175
Open
4 tasks

Difficult to read roles::test-utils::mining-device in single file #1174

rrybarczyk opened this issue Sep 30, 2024 · 3 comments · May be fixed by #1175
Assignees
Labels
roles Pertains to all roles

Comments

@rrybarczyk
Copy link
Collaborator

rrybarczyk commented Sep 30, 2024

The roles::test-utils::mining-device crate has all of its logic in a single file. There are several data types and standalone functions in this file. It can be overwhelming for a user to read in this format. After #1163 is merged, this crate should be modularized into multiple files for clarity.

  • Each data type and impl in its own file
  • Make sure all imports are imported at top of file instead of directly referenced in the logic
  • Rust doc comments
  • Add some clarity to README.md (explain how to start the local pool to get the crate to work)
@rrybarczyk rrybarczyk added the roles Pertains to all roles label Sep 30, 2024
@rrybarczyk rrybarczyk self-assigned this Sep 30, 2024
@rrybarczyk rrybarczyk changed the title Modularize roles::test-utils::mining-device Difficult to read roles::test-utils::mining-device in single file Sep 30, 2024
@rrybarczyk rrybarczyk linked a pull request Sep 30, 2024 that will close this issue
@rrybarczyk rrybarczyk linked a pull request Sep 30, 2024 that will close this issue
@Fi3
Copy link
Collaborator

Fi3 commented Oct 1, 2024

Each data type and impl in its own file

nack if we start blindly following it we will end up with millions of file and things will be much harder to understand (not to talk about import and exports). A module can contains several data type that work together. This test mining-device is very simple I would leave everything in main. If we really want to split it for sure not one file for data type. And should be very low priority.

Make sure all imports are imported at top of file instead of directly referenced in the logic

ack. Most of the time agree, some time make sense to import something in a different namespace (if we have conflicts), keep that in mind.

Rust doc comments

ack

Add some clarity to README.md (explain how to start the local pool to get the crate to work)

ack

@rrybarczyk
Copy link
Collaborator Author

Each data type and impl in its own file

nack if we start blindly following it we will end up with millions of file and things will be much harder to understand (not to talk about import and exports). A module can contains several data type that work together. This test mining-device is very simple I would leave everything in main. If we really want to split it for sure not one file for data type. And should be very low priority.

Make sure all imports are imported at top of file instead of directly referenced in the logic

ack. Most of the time agree, some time make sense to import something in a different namespace (if we have conflicts), keep that in mind.

Rust doc comments

ack

Add some clarity to README.md (explain how to start the local pool to get the crate to work)

ack

I really disagree. I don't see a downside in having multiple files. I think it breaks up the logic very nicely and the user knows exactly what to expect in each file.

I am open to other suggestions tho. What other options are you thinking of?

@Fi3
Copy link
Collaborator

Fi3 commented Oct 2, 2024

the downside is to have too many files and make thing even harder to read as I said above. Also is very common to have multiple struct that work together and to expose only one; in that case what you propose? To add a directory with a module and a file for each struct? Remember that every time that you add a directory and you nests files you make the project a little bit harder to explore. I don't think that blindly following the rule of one file per struct will bring us in a better place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
roles Pertains to all roles
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants