Skip to content
This repository has been archived by the owner on Jul 27, 2023. It is now read-only.

Extract signatures from commits, fixes #56 #84

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

alexjg
Copy link
Contributor

@alexjg alexjg commented Mar 15, 2020

No description provided.

@alexjg
Copy link
Contributor Author

alexjg commented Mar 15, 2020

Picked this one up as it looked like a good place to start getting involved. As far as I can tell the build failure is due to the build server having run out of space?

Copy link
Collaborator

@FintanH FintanH left a comment

Choose a reason for hiding this comment

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

So I'd like to keep the TryFrom instance for git2::Commit to our own Commit, so I think we shouldn't add Signature to the data structure itself.

I think the best thing to do here is adding a signature function on our version of git::Repository and git::Browser in turn could call that function. On top of that, keeping the same interface of the function would be cool, I think signature_field will become useful if we plan on allowing other types of commit signatures.

So I envision the use of it as something like:

let signature = browser.signature(commit.id, Some("rad"))?;

I hope this helps and thanks for the PR! :)

@FintanH
Copy link
Collaborator

FintanH commented Mar 16, 2020

As far as I can tell the build failure is due to the build server having run out of space?

Ya it looks like it. I'll figure this out for you in the meantime :)

@alexjg
Copy link
Contributor Author

alexjg commented Mar 16, 2020

I went for browser.extract_signature for the name, just for consistency with the git naming, does that work?

@FintanH
Copy link
Collaborator

FintanH commented Mar 16, 2020

I went for browser.extract_signature for the name, just for consistency with the git naming, does that work?

Makes total sense 👌

Ya it looks like it. I'll figure this out for you in the meantime :)

We figured out the cache volume issue. Just need to put a fix in place :)

Copy link
Collaborator

@FintanH FintanH left a comment

Choose a reason for hiding this comment

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

This is awesome! There's one last thing I want to do. We should add a couple of unit tests.

We use https://github.com/radicle-dev/git-platinum as a golden file suite. So we could add doc test, the same as the rest where we look at two different commits to extract the signature. Currently git-platinum has branch protection with signed commits only. So let's just do the positive case for now, meanwhile I'll try and set up git-platinum with a case that has no signature on a commit :)

@alexjg
Copy link
Contributor Author

alexjg commented Mar 16, 2020

Ah I wondered what all that git-platinum stuff was. Doctest added.

@FintanH
Copy link
Collaborator

FintanH commented Mar 16, 2020

Ah I wondered what all that git-platinum stuff was. Doctest added.

Amazing! Sorry for the back-and-forth, here's my last request I PROMISE. We have a commit in git-platinum now that doesn't have a signature: radicle-dev/git-platinum@80bacaf so if you could test that out we'll be all gravy.

Also, I'd like to move the doc tests to use this form https://rust-lang.github.io/api-guidelines/documentation.html#examples-use--not-try-not-unwrap-c-question-mark. So if you could use this form for the one you're adding that'd be awesome :)

@alexjg
Copy link
Contributor Author

alexjg commented Mar 16, 2020

No problem, I actually really enjoy the back and forth, it's a similar feeling to the feeling of polishing a finished piece of woodwork, like making everything perfect 🙂

Negative doctest and ? example added.

Copy link
Collaborator

@FintanH FintanH left a comment

Choose a reason for hiding this comment

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

Amazing, thank you! I'm proud of our finished woodwork and glad you enjoyed the experience ❤️

I just need to finish figuring out this build issue ;)

@FintanH
Copy link
Collaborator

FintanH commented Mar 16, 2020

For some reason, this build isn't getting picked up here. But since there's one build on CI passing I'm happy to merge this :)

@FintanH FintanH merged commit 0063764 into radicle-dev:master Mar 16, 2020
@FintanH FintanH mentioned this pull request Mar 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants