Skip to content

Conversation

@TheSnoozer
Copy link
Collaborator

Hi @ktoso,
today I had some time to implement the feature for #54.
This PR looks huge but it isn't.
Most what I did was shifting code from DescribeCommand.java to a new class JGitCommon.java to make it easier accessible. This basically gave me the opportunity to reuse the major part of the functionality. We still could speed up the JGit-Implementation by saving the values we need twice but I leave this as optional :-9

Also I fixed the license-headers (some files had a duplicated license info) and added the new properties to the readme (I caught one that was missing...).

Let me know if I should improve something for the PR.

Thanks,

@TheSnoozer TheSnoozer added this to the 2.1.15 milestone May 24, 2015
@ktoso
Copy link
Collaborator

ktoso commented May 24, 2015

Sounds great, reviewing!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove all @author tags by the way, to make nier to feel collective ownership about the code. I'll do that. :-)

@TheSnoozer
Copy link
Collaborator Author

Well I think we could make it static.
In one of my projects I learned the hard way to avoid static method calls and rather go for new Clazz().method ;-)
I think this memory is burned into my head :-)

@ktoso
Copy link
Collaborator

ktoso commented May 25, 2015

Or better just pass the instance around ;)

@TheSnoozer
Copy link
Collaborator Author

Mhh I think I would leave it as it is right now :-)
Passing only a single instance around or making the calls static is something I would leave optional for further improvement :-P

@ktoso
Copy link
Collaborator

ktoso commented May 27, 2015

Though I would much rather have it passed in from the "beginning of the control flow" (the plugin, or the test) let's go with the current impl for now ;-)

ktoso added a commit that referenced this pull request May 27, 2015
#54: Reveal describe details (tag name, number of commits)
@ktoso ktoso merged commit 8531891 into git-commit-id:master May 27, 2015
@ktoso
Copy link
Collaborator

ktoso commented May 27, 2015

Thanks, merged!

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.

2 participants