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

Expose the commit dates #300

Merged
merged 1 commit into from
Oct 24, 2016
Merged

Expose the commit dates #300

merged 1 commit into from
Oct 24, 2016

Conversation

stephenc
Copy link
Contributor

I need this for SCM API stuff

@kohsuke kohsuke merged commit 8b34696 into hub4j:master Oct 24, 2016
Copy link
Contributor

@KostyaSha KostyaSha left a comment

Choose a reason for hiding this comment

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

Smells like copy-paste bug

@@ -42,11 +42,19 @@ public GitUser getAuthor() {
return author;
}

public Date getAuthoredDate() {
return GitHub.parseDate(author.date);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why authored? It's AuthorDate

@WithBridgeMethods(value = GHAuthor.class, castRequired = true)
public GitUser getCommitter() {
return committer;
}

public Date getCommitDate() {
return GitHub.parseDate(author.date);
Copy link
Contributor

Choose a reason for hiding this comment

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

So what is the difference of this two methods? Copy-paste instead commit.date?

@KostyaSha
Copy link
Contributor

@kohsuke did you review it or blindly merged and released?

@KostyaSha
Copy link
Contributor

@stephenc ?

@kohsuke
Copy link
Collaborator

kohsuke commented Oct 25, 2016

Good catch! I did look at the change but I missed it.

I'll fix this and push out a new release.

@kohsuke
Copy link
Collaborator

kohsuke commented Oct 25, 2016

I'll leave the getAuthoredDate as is. I trust Stephen's English than mine and yours.

@stephenc
Copy link
Contributor Author

Good catch on the copy and paste.

So I went with "commitDate" rather than "committerDate" and "authoredDate" vs "authorDate" as these are the dates of the respective actions. In a sense GitHub conflates the action with the person, and it just sounded wrong to my ear to say "getCommitterDate"... what's the committer date... does that method arrange for flowers a movie and a meal with the committer... ok, perhaps the committer deserves that as a reward for their work... that is more the date of the commit... closest analogy for author I could find was authored

@KostyaSha
Copy link
Contributor

Question not in english, it not a novel. When you know API you can use library easily, otherwise you should scroll code/methods to get idea what they return. Also why you can't call getter().field yourself and need this proxy method?

@stephenc stephenc deleted the commit-dates branch November 1, 2017 15:50
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.

3 participants