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

Move more definitions to the geiger crate #117

Closed
stephaneyfx opened this issue Sep 28, 2020 · 9 comments
Closed

Move more definitions to the geiger crate #117

stephaneyfx opened this issue Sep 28, 2020 · 9 comments
Labels
enhancement New feature or request important If you want to contribute, please consider this issue before others.

Comments

@stephaneyfx
Copy link
Contributor

It would be helpful to move more definitions from the bin crate cargo-geiger to the lib crate geiger so that they can be used by integration tests (e.g. to deserialize the JSON output) and other crates. However many type definitions use PackageId which is defined in the cargo crate. The geiger crate does not depend on cargo and states the following in its description:

Some library parts of cargo-geiger, decoupled from cargo

Is it ok to make geiger depend on cargo given that such core definitions come from cargo?

@Shnatsel
Copy link

Making struct definitions public is a good thing! However, we're trying to move away from depending on cargo directly and use cargo metadata instead, see #16

@stephaneyfx
Copy link
Contributor Author

Thank you for your answer. I will take a look at cargo-metadata. We're trying to use cargo-geiger in our CI and with the json output recently merged, we are hoping to experiment with allow-lists but would rather not duplicate the type definitions needed to deserialize.

@anderejd
Copy link
Contributor

We could probably start using the PackageId from cargo_metadata for the json serialization as a first step towards removing the dependency on the cargo library crate. Another option would be to define our own version of PackageId, at geiger::PackageId.

The former would avoid defining our own PackageId, possibly causing confusion. The latter would avoid adding another dependency to the geiger crate. I'm not sure which one I prefer.

Can you think of other solutions?

@Shnatsel
Copy link

PackageId from cargo_metadata crate is conceptually a black box with Eq trait implementation. So I'd say roll our own; there is not much value in reusing the one from cargo_metadata, and if we do then any semver change for cargo_metadata crate would necessitate a breaking change for geiger crate.

@anderejd
Copy link
Contributor

Alright, let's define our own geiger::PackageId and possibly other types needed for the json serialization, and let's define them in the geiger crate.

Alternatively, we could make a new crate cargo_geiger_json, that contains all types needed to serialize/deserialize the json format produced by cargo-geiger, but that could be my modularization/loose coupling OCD kicking in. Opinions?

@anderejd anderejd added the question Further information is requested label Sep 29, 2020
@Shnatsel
Copy link

It sounds good to me to depend on a lightweight crate if you only want to parse the JSON. That's why we have cargo-metadata crate decoupled from cargo

@anderejd
Copy link
Contributor

anderejd commented Sep 30, 2020

Then a loosely coupled lightweight crate it is.

TODO:

  • Create a dedicated lightweight crate for data types used by the recently added json output format, let's call it something like cargo-geiger-json or maybe cargo-geiger-serde?.

@anderejd anderejd added enhancement New feature or request important If you want to contribute, please consider this issue before others. and removed question Further information is requested labels Sep 30, 2020
@anderejd
Copy link
Contributor

anderejd commented Oct 2, 2020

I reserved a crate name: https://crates.io/crates/cargo-geiger-serde
Will merge the new empty crate to the master branch in a moment.

anderejd added a commit that referenced this issue Oct 2, 2020
anderejd added a commit that referenced this issue Oct 2, 2020
stephaneyfx added a commit to stephaneyfx/cargo-geiger that referenced this issue Oct 15, 2020
@anderejd
Copy link
Contributor

Fixed by #125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request important If you want to contribute, please consider this issue before others.
Projects
None yet
Development

No branches or pull requests

3 participants