Jump to content

My First Pull Request


jeffryfisher

Recommended Posts

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?

The debugger is a powerful and necessary tool in any IDE, so learn how to use it. You'll be able to tell us more and get better help here if you investigate your runtime problems in the debugger before posting.

Link to comment
Share on other sites

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?

The debugger is a powerful and necessary tool in any IDE, so learn how to use it. You'll be able to tell us more and get better help here if you investigate your runtime problems in the debugger before posting.

Link to comment
Share on other sites

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.

The debugger is a powerful and necessary tool in any IDE, so learn how to use it. You'll be able to tell us more and get better help here if you investigate your runtime problems in the debugger before posting.

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.
Note: Your post will require moderator approval before it will be visible.

Guest
Unfortunately, your content contains terms that we do not allow. Please edit your content to remove the highlighted words below.
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Announcements



×
×
  • Create New...

Important Information

By using this site, you agree to our Terms of Use.