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

Implement, test, doc git descriptions for things #20885

Merged
merged 4 commits into from
May 18, 2017
Merged

Implement, test, doc git descriptions for things #20885

merged 4 commits into from
May 18, 2017

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Mar 3, 2017

Now you can ask for detailed descriptions of a repo's workdir or a commit-like object within it. cc @staticfloat.

@kshyatt kshyatt added the libgit2 The libgit2 library or the LibGit2 stdlib module label Mar 3, 2017

* `options::DescribeOptions=DescribeOptions()`

Equivalent to `git-describe <commitish>`.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there should be a hyphen between git and describe; git describe is the CLI AFAICT

@ararslan
Copy link
Member

ararslan commented Mar 3, 2017

It might be nice to have the description interface be simply LibGit2.describe(commitish, ...). That's a little more clear IMO than having to directly construct a GitDescribeResult.

@kshyatt
Copy link
Contributor Author

kshyatt commented Mar 3, 2017

I did it this way because it's a type and otherwise it lacks a constructor.

@ararslan
Copy link
Member

ararslan commented Mar 3, 2017

describe could just be an alias for the constructor

test/libgit2.jl Outdated
@@ -354,6 +354,7 @@ mktempdir() do dir
@test contains(showstr[4], "SHA:")
@test showstr[5] == "Message:"
@test showstr[6] == commit_msg1

Copy link
Contributor

Choose a reason for hiding this comment

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

don't add empty lines as the first or last thing in a block

@tkelman
Copy link
Contributor

tkelman commented Mar 4, 2017

we're in feature freeze, not the best time to be wrapping new api's

@kshyatt
Copy link
Contributor Author

kshyatt commented Mar 4, 2017

@staticfloat asked for it, we can wait until we're out of FF to merge if you want.

@kshyatt
Copy link
Contributor Author

kshyatt commented May 4, 2017

@tkelman I've rebased this against master. We're off feature freeze, right?

@kshyatt
Copy link
Contributor Author

kshyatt commented May 10, 2017

And bumping again. Can someone ( @ararslan ? @simonbyrne ? ) please review this?

Copy link
Contributor

@simonbyrne simonbyrne left a comment

Choose a reason for hiding this comment

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

I made the small doc change suggested by @ararslan, LGTM once CI is done.


* `options::DescribeOptions=DescribeOptions()`

Equivalent to `git-describe`.
Copy link
Contributor

Choose a reason for hiding this comment

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

does the same apply here?

@kshyatt kshyatt merged commit cd4b4b8 into master May 18, 2017
@kshyatt kshyatt deleted the ksh/describe branch May 18, 2017 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants