Skip to content

Make this crate into a library as well. #2

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

Merged
merged 4 commits into from
Sep 22, 2020

Conversation

nohupped
Copy link
Contributor

@nohupped nohupped commented Sep 21, 2020

Hello,
Thank you for this crate. We have a requirement in one of our project where we could use this as a library dependency. I've moved the core functions and the related tests that you've written in your main.rs into a library so that we can use it from the https://crates.io. None of the core functionalities were altered, only a refactor to use this as a library.
Currently, we are using this in our cargo.toml as

json_diff = { git = "https://github.com/nohupped/json-diff" }

It'd be great if you can consider reviewing this PR.

  • Define [lib] and [[bin]] sections in the Cargo.toml file to keep this a binary, while also supporting to be a library
  • Moved top level functions from main.rs to lib.rs and changed scope to pub so that this can be called from other crates.
  • Moved the unit tests that were relevant to the moved functions into lib.rs
  • To give the user a chance to handle the error instead of a panic from the library, changed the return type of compare_jsons and display_output functions to type Result and return error in favour of unwrap(), and made the changes in main.rs while calling these functions.
    Fixes available as a library? #1
    Thanks.

@nohupped nohupped changed the title Move the compare_jsons and display_output functions to a library Make this crate into a library as well. Sep 21, 2020
Copy link
Owner

@ksceriath ksceriath left a comment

Choose a reason for hiding this comment

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

Thanks for you contribution.
I have looked at the changes partially. I'll go through the rest hopefully later today.

* removed a few unwraps in favour of Result inside the lib
src/lib.rs Outdated
Ok(process::match_json(&value1, &value2))
}

pub fn display_output(result: Mismatch) -> Result<(), std::io::Error> {
Copy link
Owner

Choose a reason for hiding this comment

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

display_output writes to the stdout. This responsibility should not lie with the library.
Do you think it'll be better to refactor the 'writing to stdout' part out of this and into the binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with what you said about display_output being part of the binary. compare_jsons should be good enough in my opinion. I will move it.

src/lib.rs Outdated
Comment on lines 11 to 12
let value1 = serde_json::from_str(a)?;
let value2 = serde_json::from_str(b)?;
Copy link
Owner

Choose a reason for hiding this comment

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

We're losing the specificity of the error here - this is helpful to point out to user exactly where the error happens. The Message constants are there to specify the exact errors this crate can run into and report those.
If you think the exact errors returned by the serde_json are important (or useful), can you see if the Message can be re-purposed to hold an Error, but still have a useful message on top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. From my requirement's perspective, it isn't much of an importance, but as a library, it is a good to have. I will accommodate that.

@nohupped
Copy link
Contributor Author

The requested changes are added. I've added a derive PartialEq to the enum for the sake of running an assert_eq on the enums on the test cases. If you think this is not required, I can remove that.

@nohupped nohupped requested a review from ksceriath September 22, 2020 17:05
@ksceriath ksceriath merged commit b6b6453 into ksceriath:master Sep 22, 2020
@nohupped
Copy link
Contributor Author

Thank you very much @ksceriath

@nohupped
Copy link
Contributor Author

nohupped commented Sep 22, 2020

@ksceriath Would you be uploading this into http://crates.io/, or should we use it as json_diff = { git = "https://github.com/ksceriath/json-dif" } in our project?

@ksceriath
Copy link
Owner

I've published v0.1.2 on crates.io

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.

available as a library?
2 participants