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

Added P2P tunnel part for OpenComputers. #579

Closed
wants to merge 6 commits into
base: rv2
from

Conversation

Projects
None yet
3 participants
@fnuecke
Contributor

fnuecke commented Dec 11, 2014

As discussed with @FireBall1725 on IRC, here's a PR adding a P2P tunnel part for OpenComputers networking.

Tested with and without OC, in dev-env and normal MC via vanilla launcher.

One thing that needs specific review are the adjusted enums. I believe the ones where I did not append the entry to the end to be order irrelevant, but verification of that is needed.

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Dec 11, 2014

Member

I would consider the API and localization currently under lockdown until it is finally merged. Otherwise it just makes things more complicated.

The code style does not look like it is matching our.
For example we do not prefix fields with _ (this is not PHP or Python and missing visibility), the import order is wrong and is there a reason why you are using fqdns for the classes?

Member

yueh commented Dec 11, 2014

I would consider the API and localization currently under lockdown until it is finally merged. Otherwise it just makes things more complicated.

The code style does not look like it is matching our.
For example we do not prefix fields with _ (this is not PHP or Python and missing visibility), the import order is wrong and is there a reason why you are using fqdns for the classes?

@fnuecke

This comment has been minimized.

Show comment
Hide comment
@fnuecke

fnuecke Dec 11, 2014

Contributor

Made the requested changes.

I would consider the API and localization currently under lockdown until it is finally merged.

Any ETA on that? Just so I know whether to keep my hacky third-party implementation in OC itself for now.

Contributor

fnuecke commented Dec 11, 2014

Made the requested changes.

I would consider the API and localization currently under lockdown until it is finally merged.

Any ETA on that? Just so I know whether to keep my hacky third-party implementation in OC itself for now.

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Dec 13, 2014

Member

resync please

Member

thatsIch commented Dec 13, 2014

resync please

@fnuecke

This comment has been minimized.

Show comment
Hide comment
@fnuecke

fnuecke Dec 13, 2014

Contributor

Cool, done! Also re-tested for good measure.

Contributor

fnuecke commented Dec 13, 2014

Cool, done! Also re-tested for good measure.

@thatsIch

View changes

Show outdated Hide outdated src/main/java/appeng/integration/modules/OC.java
@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Dec 13, 2014

Member

Looks fine to me, if you redo the import orders. codeformat/consistent.importorder has a priority list map

Member

thatsIch commented Dec 13, 2014

Looks fine to me, if you redo the import orders. codeformat/consistent.importorder has a priority list map

@fnuecke

This comment has been minimized.

Show comment
Hide comment
@fnuecke

fnuecke Dec 14, 2014

Contributor

consistent.importorder

Ah, missed that, importing the code style only showed XML files, so I just oriented myself on other files for the import order. Will take a look.

Contributor

fnuecke commented Dec 14, 2014

consistent.importorder

Ah, missed that, importing the code style only showed XML files, so I just oriented myself on other files for the import order. Will take a look.

@fnuecke

This comment has been minimized.

Show comment
Hide comment
@fnuecke

fnuecke Dec 14, 2014

Contributor

Updated.

Contributor

fnuecke commented Dec 14, 2014

Updated.

@fnuecke

This comment has been minimized.

Show comment
Hide comment
@fnuecke

fnuecke Dec 14, 2014

Contributor

Aaand, the one time I push while testing it and not after it breaks... the instance field seems to be required. Doesn't work if I remove it (so I added it back).

Contributor

fnuecke commented Dec 14, 2014

Aaand, the one time I push while testing it and not after it breaks... the instance field seems to be required. Doesn't work if I remove it (so I added it back).

@thatsIch

View changes

Show outdated Hide outdated src/main/java/appeng/integration/IntegrationType.java
@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Dec 16, 2014

Member

We will pull it into the rv3 branch

Member

thatsIch commented Dec 16, 2014

We will pull it into the rv3 branch

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch May 5, 2015

Member

we would pull this into rv3 now,
not sure if anything particular new was added on your side.

Else I can do the rabase on it.

Member

thatsIch commented May 5, 2015

we would pull this into rv3 now,
not sure if anything particular new was added on your side.

Else I can do the rabase on it.

thatsIch and others added some commits May 5, 2015

@fnuecke

This comment has been minimized.

Show comment
Hide comment
@fnuecke

fnuecke May 6, 2015

Contributor

Great! I'll update it to the current state of the rv2 branch and check if it's still working.

Contributor

fnuecke commented May 6, 2015

Great! I'll update it to the current state of the rv2 branch and check if it's still working.

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch May 6, 2015

Member

rebase it against the master (this will be the future rv3)

Member

thatsIch commented May 6, 2015

rebase it against the master (this will be the future rv3)

@fnuecke

This comment has been minimized.

Show comment
Hide comment
@fnuecke

fnuecke May 7, 2015

Contributor

Oops, messed that up. Will make a new PR against master.

Contributor

fnuecke commented May 7, 2015

Oops, messed that up. Will make a new PR against master.

@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch May 7, 2015

Member

welcome to github :D

Member

thatsIch commented May 7, 2015

welcome to github :D

@fnuecke

This comment has been minimized.

Show comment
Hide comment
@fnuecke

fnuecke May 7, 2015

Contributor

Haha, yeah. I pretty much expected that to horribly fail, admittedly :P

Contributor

fnuecke commented May 7, 2015

Haha, yeah. I pretty much expected that to horribly fail, admittedly :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment