> General questions:<br>
> 1. Does installFacet() method needs to be called anywhere in the Git plugin?<br><br>Only in the SetupCommand, which is already done :)<br><br>
> 2. Does Git plugin should be split into two plugins? One containing<br>
> project-related methods, and another, containing methods, which could<br>
> be called outside of a project.<br><br>Not at the moment. <br><br>For now, I would <b>not</b> worry about writing the Git plugins themselves. I would start with the UndoPlugin and the GitFacet functionality. We can worry about GitPlugin later, since we already have passthrough support for Git via the GitShellPlugin.<br>
<br>Sensible?<br>~Lincoln<br><br><div class="gmail_quote">On Tue, May 22, 2012 at 8:14 AM, Jevgeni Zelenkov <span dir="ltr"><<a href="mailto:jevgeni.zelenkov@googlemail.com" target="_blank">jevgeni.zelenkov@googlemail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Forge developers,<br>
<br>
I am still stuck trying to get FORGE-183 pull request accepted. Did I<br>
set the record already as the longest unaccepted pull request in the<br>
Forge's history? :)<br>
<br>
Here is the summary of all the feedback I received so far:<br>
<br>
----<br>
----<br>
Does it make sense to require the Git Facet on the GitPlugin? Facets<br>
imply that a project is also required, and it may be possible to use<br>
GitPlugin outside of a project. This is a limitation, currently, where<br>
Facet constraints cannot be placed on individual methods - but<br>
something we hope to address in the future. So in the mean time, The<br>
plugin would need to be split into two separate plugins - we can<br>
discuss the best approach for this on the dev list.<br>
--<br>
Regarding the GitFacet - I believe it's in the right place for the moment,<br>
but I don't believe that the GitPlugin should be referencing it, since the<br>
commands in GitPlugin could be used even without a Project.<br>
<br>
GitFacet (as the issue describes poorly) is something that should be usable<br>
to inspect the state of a Project via:<br>
<br>
project.hasFacet(GitFacet.class)<br>
<br>
After that, everything else is an added benefit. It should probably also<br>
perform tasks like:<br>
<br>
project.getFacet(GitFacet.class).getBranches()<br>
project.getFacet(GitFacet.class).getTags()<br>
project.getFacet(GitFacet.class).getRemotes()<br>
project.getFacet(GitFacet.class).getStatus()<br>
<br>
And that's where your imagination kicks in.<br>
--<br>
To confuse things even further, I would not implement the actual JGit<br>
features in the Facet itself (even though it would be correct to do<br>
so.) I would instead create a utility class that the Facet itself<br>
would use. This utility class could be reused by plugins directly (for<br>
plugins that do not necessarily operate on a project.)<br>
<br>
So, really FORGE-183 is open-ended, but in general it's meant to allow<br>
projects to know when they contain a Git repository. You will need this<br>
feature for the Git undo plugin.<br>
--<br>
My recommendation is to implement GitFacet in such a way that it will be<br>
"installed" or "registered" if the project contains, or exists in, a git<br>
repository. Then once you have done that, I'd start working on the<br>
UndoPlugin and UndoFacet.<br>
----<br>
----<br>
<br>
<br>
Based on that feedback, I wrote a TODO list of updates, which should<br>
be added to the current pull request to get it accepted:<br>
<br>
1. remove "RequiresFacet" annotation from the Git plugin<br>
2. At least the following methods should be added to the Git plugin,<br>
GitFacet and GitUtils: getBranches(), getTags(), getRemotes() and<br>
getStatus(). GitFacet has no dependency on jGit, GitUtils is dependent<br>
on jGit. GitFacet install() and isInstalled() methods are OK in their<br>
current form.<br>
3. Current GitUtils methods should refactored, so that they don't<br>
return any objects from jGit library, like Ref. (the only exception<br>
being "public static Git git(final DirectoryResource dir)")<br>
<br>
Does that make sense?<br>
<br>
Here is a demo implementation of the getBranches methods for Git<br>
plugin, GitFacet and GitUtils. Other methods will look similar.<br>
<br>
Git plugin method implementation:<br>
<br>
@Command("branches")<br>
public void gitGetBranches() throws Exception<br>
{<br>
if (shell.getCurrentProject().hasFacet(GitFacet.class))<br>
shell.getCurrentProject().getFacet(GitFacet.class).getBranches();<br>
}<br>
<br>
<br>
GitFacet method implementation:<br>
<br>
public List<String> getBranches()<br>
{<br>
Git git = GitUtils.git(project.getProjectRoot());<br>
return GitUtils.getBranches(git);<br>
}<br>
<br>
<br>
GitUtils method implementation (actually implements the functionality):<br>
<br>
public static List<String> getBranches(final Git repo)<br>
{<br>
List<String> results = new ArrayList<String>();<br>
<br>
for(Ref branchRef : repo.branchList().call())<br>
{<br>
results.add(branchRef.getName());<br>
}<br>
<br>
return results;<br>
}<br>
<br>
<br>
General questions:<br>
1. Does installFacet() method needs to be called anywhere in the Git plugin?<br>
2. Does Git plugin should be split into two plugins? One containing<br>
project-related methods, and another, containing methods, which could<br>
be called outside of a project.<br>
<br>
I would appreciate your feedback.<br>
<br>
<br>
Best regards,<br>
Jevgeni<br>
_______________________________________________<br>
forge-dev mailing list<br>
<a href="mailto:forge-dev@lists.jboss.org">forge-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/forge-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/forge-dev</a><br>
</blockquote></div><br><br clear="all"><br>-- <br>Lincoln Baxter, III<br><a href="http://ocpsoft.org" target="_blank">http://ocpsoft.org</a><br>"Simpler is better."<br>