Skip to content
This repository has been archived by the owner on Feb 1, 2019. It is now read-only.

Node main function is not designed or written well #71

Open
jamesray1 opened this issue May 15, 2018 · 4 comments
Open

Node main function is not designed or written well #71

jamesray1 opened this issue May 15, 2018 · 4 comments
Labels
M4: core Core client code / Rust.

Comments

@jamesray1
Copy link
Member

Is your feature request related to a problem? Please describe.
The node crate has a main function which only contains functionality for building the UML diagram. This is problematic because it doesn't adequately link to the rest of the functionality in the crate, and there is no error handling, which makes it very difficult to fix errors when a panic occurs, as has happened a few times now, and then I have had to spend hours trying to find the cause of the error by trial and error. Additionally, this also points to the need to do test-driven development, which may seem to be harder, but may be worth it in the long run.

Describe the solution you'd like
Write a new main function and rename the current one to make_uml_diagram. Strive to do TDD from now on; it will probably only get harder the longer we put off doing that.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

@jamesray1 jamesray1 added the M4: core Core client code / Rust. label May 15, 2018
@jamesray1
Copy link
Member Author

7a32c61 is an example of this problem, cargo test and cargo build panic, and it is very hard to find and fix the root cause. I have used git diff 128b7c8 to compare with the previous commit, and after comparing the diff multiple times, I have not been able to fix the error.

@ltfschoen
Copy link
Collaborator

Generation of the UML diagram is simply an implementation of this library https://github.com/adjivas/ml, as specified on Line 1 of build.rs https://github.com/Drops-of-Diamond/diamond_drops/blob/develop/node/build.rs#L1. The "Usage" instructions of that crate were followed https://github.com/adjivas/ml#usage, hence why a main() function exists in build.rs.

If you desperately need to temporarily disable use of that build.rs file and the associated crate that generates the UML diagram to isolate it and ensure it isn't the cause of any errors that may arise due to new changes made to the codebase, then you would just comment out Line 6 https://github.com/Drops-of-Diamond/diamond_drops/blob/develop/node/Cargo.toml#L6 and Line 15-16 https://github.com/Drops-of-Diamond/diamond_drops/blob/develop/node/Cargo.toml#L15 in /node/Cargo.toml

I agree it would be good to refactor the contents of the main function into a separate function with a meaningful name as you've mentioned make_uml_diagram, i.e.

fn make_uml_diagram() {
    // Generate diagram in /diagrams/ml.svg for README.md
    let dest: String = "../diagrams/".to_string();
    let _ = mml::src2both("src", dest.replace("-", "_").as_str());

    // Generate diagram in /target/doc/diamond_drops_node/ml.svg
    let dest: String = concat!("../target/doc/", env!("CARGO_PKG_NAME")).to_string();
    let _ = mml::src2both("src", dest.replace("-", "_").as_str());
}

fn main() {
    make_uml_diagram();
}

I agree that additional error handing (using error_chain that we're now using) and meaningful message could be added to make it clear if the UML library or its associated implementation is the cause of a panic.

@jamesray1
Copy link
Member Author

Thanks for the tip! It wasn't due to build.rs and MML, but your suggestion helped to find and fix the error! I have also modified this in blob-serialization-6, although I'll checkout the changes to another branch and push from there, then that can be merged once 5 is merged.

@jamesray1
Copy link
Member Author

The above refactor is added in #73.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
M4: core Core client code / Rust.
Projects
None yet
Development

No branches or pull requests

2 participants