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

refactor: separate dsc into a library and a binary crate #6

Merged
merged 5 commits into from
Nov 12, 2022

Conversation

EricCrosson
Copy link
Contributor

The motivation for this change is so that I can import the engine of docker-source-checksum as a function in some other binary, without introducing changes to current docker-source-checksum users.

@EricCrosson
Copy link
Contributor Author

Marking as "ready for review" because from what I can tell, this is working as expected! But it may be in what you consider to be a rough state, happy to iterate on this as feedback is presented

@EricCrosson EricCrosson marked this pull request as ready for review November 11, 2022 16:54
@dpc
Copy link
Owner

dpc commented Nov 11, 2022

@EricCrosson All good, but I think underscores vs hypens are super confusing. Would putting the binary and library in the same crate (like e.g. here: https://github.com/crev-dev/cargo-crev/blob/e9b35a1d24a7b877bf20d19632e8ef9f04b210ac/cargo-crev/Cargo.toml#L17) work for you?

The motivation for this change is so that I can import the engine
of docker-source-checksum as a function in some other binary,
without introducing changes to current docker-source-checksum users.
@EricCrosson
Copy link
Contributor Author

I think underscores vs hypens are super confusing

Agreed! There may still be a bit of that, but I followed the example you linked and the diff is much cleaner. Thanks for the pointer

Copy link
Contributor

@ocornoc ocornoc left a comment

Choose a reason for hiding this comment

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

I have some criticisms of this code (was requested privately by @EricCrosson).

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@ericcrosson-bitgo
Copy link

Thanks for the input @ocornoc. I'd like to keep this PR as close to a lift-and-shift as possible, but I'm happy to circle back with these suggestions

dpc and others added 4 commits November 11, 2022 17:01
Co-authored-by: Grayson Burton <ocornoc@protonmail.com>
Co-authored-by: Grayson Burton <ocornoc@protonmail.com>
Co-authored-by: Grayson Burton <ocornoc@protonmail.com>
Co-authored-by: Grayson Burton <ocornoc@protonmail.com>
@dpc
Copy link
Owner

dpc commented Nov 12, 2022

@EricCrosson Proposed changes looked good so I added them. Please ping me if you're OK with my landing as is.

@EricCrosson
Copy link
Contributor Author

@dpc Looks good to me! Thank you

@dpc dpc merged commit 6b76e8b into dpc:master Nov 12, 2022
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.

None yet

4 participants