My First Pull Request

[jeffryfisher]

I've taken the plunge into MinecraftForge at GitHub. However, being unfamiliar with git, I am unclear on the process. Not wanting to screw up anything, I first forked 1.10.x. I then edited two of Forge's classes. Each produced a "pull request", but only within my fork.

 

Still not wanting to screw up Forge, I have not tried to merge anything. I am unsure what (if anything) to do next.

 

My work is at https://github.com/jeffryfisher/MinecraftForge

 

Should I merge, or should I let others take it from here?

[jeffryfisher]

First of all, what you have created are branches, not pull-requests.

Well, I was right about not understanding git and it's web UI.

 

As such, this does not make much sense, since the two commits you made to individual branches should really be on one branch, in one commit, since they depend on each other.

Agreed. It was the UI that railroaded me into saving each edit like that. I'll try again to see if there's a way for git to save each edit on the same branch before committing, but the web UI didn't seem to offer me any choices yesterday.

 

If you want to actually create a pull-request now (which by the way is nothing more than a "please merge this" request) you use the "New pull request" button next to the branch selector on Github and select which of your branches you would like to see in main forge.

I wonder if it might be better for me to post a suggestion (and half-baked code) in the suggestion forum so someone with better tools and more experience can do it right. Let me know if I should take that approach.

 

As for your changes, you are breaking binary compatibility since you are changing a constructor, without providing a fallback for the old version.

So, even if it seems as if the only instantiator is the other file I edited, some mod out there might instantiate its own event object and break if my change hit it? I guess the original constructor can call the new one and pass null as an argument to the new player parameter.

 

Also, I don't think this is intended.

 

Aha, that's why it thought I had 3 changes! I have no idea how I managed to paste that there.

 

I can also highly recommend not using the Web-UI to change code.

 

I've reached that same conclusion myself. I'll see what happens in Eclipse when I try to work on my own copies of files that duplicate class names inside the libraries.

 

As for "not wanting to screw up forge": You cannot. You don't have access to the forge repo, hence you cannot break it. You can only ask for your changes to be merged back, that is a pull request.

OK, so after fixing a couple things, I should try a merge. If I merge in the right order, then my branches should reduce to one, and then I might make a proper PR.

 

My other quandary is whether or not I should try to wade into the third part of the change: Fixing the hook's call from inside the vanilla class to pass its "this.thePlayer" field to the modified Forge method. I don't even know where to look for the script that injects Forge hooks into vanilla classes. Would you prefer that I leave that to someone else, or should I learn where to get it done?

[jeffryfisher]

Well... With dev environment setup looking like more work than the coding itself, I have wimped out. I posted an "issue" in github along with the code for the two Forge parts that I semi-validated in Eclipse. I am hoping that someone with more experience and better tools can take it across the finish line.