search

echurchill / CityWorld

Minecraft/Bukkit world generator
62 stars 36 forks source link

Upgrade to Spigot 1.13.2 (Not 100% yet) #28

Closed jyhsu2000 closed 5 years ago

jyhsu2000 commented 5 years ago

Hello, @echurchill and someone which looking for a new version. This is the most awesome world generator I have seen. I can't wait for the version for 1.13.2. So I spend several days on fixing compatibility of 1.13.2.

I don't know whether it is helpful for you. And it's not 100% fixed yet. Just send a PR to let you know somebody is working on it.

It's a huge jobs, so I cannot describe all the detail I do. Just know, this version can run on Spigot/PaperSpigot 1.13.2 And the following is some major points I fixed.

I'm still working on it. (It's a major plugin in my server and this plugin is really amazing)

I'm not sure that you will notice this PR or not. I just want to make some contribution for it within my ability.

Exactly, I can only fix some bug so far, but we still wish for making some more awesome terrain/biome in the new version. We know everyone has a job in the real world, but we'll be glad if you are still or will be back to work on this plugin. Thanks again for making this amazing plugin.

RoboMWM commented 5 years ago

But muh commit authorship why didn't you just branch off of my PR :c

jyhsu2000 commented 5 years ago

But muh commit authorship why didn't you just branch off of my PR :c

I am sorry for my carelessness. At first, I tried to find your repo in the PR page. But I cannot access the repo/branch from View file button in Files changed page of your PR. And trying to find it in your profile directly, but in vain. I only found another world generator called CyberWorld. When I finally noticed it's under another namespace MLG-Fortress, I have already done many things.

RoboMWM commented 5 years ago

As well as the commits themselves...

RoboMWM commented 5 years ago

Anyways, this PR has a ton of junk in it, intellij files that shouldn't be in there and etc. Diff is basically unreadable, so much so that github won't even load the list of files that have changed, so I can't even tell how much extraneous junk (like compiled classes and etc.) has been committed vs. the real source code diffs. Maybe a lot of it is just a ton of whitespace changes too, since iirc this codebase uses tabs, not spaces for indenting.

jyhsu2000 commented 5 years ago

Clickable text doesn't show to user who doesn't have push permission. And the other one is the one I didn't notice at that time.

image

About code style, I didn't find any hints, project settings or a .editorconfig file for defining code style. And I found some code was not well formated at that time. (e.g. different/wrong indent in some files, trailing spaces in some lines) So I just used default code formatter provided by IntelliJ IDEA. I have changed indent back to TAB and add a .editorconfig file to define it.

About some files should/shouldn't be traced in repo, I use gitignore.io to generate .gitignore file. I think it already filters out file not need to be traced. Not all the files about IntelliJ IDEA shouldn't traced. The only file shouldn't be traced but it's traced now is PhatLoots.jar. Because it doesn't provide maven repo and just to let everyone can compile it with correct version easily. If the author thinks it shouldn't be traced, I'll also take it off.

If you think whitespace make Diff page hard to read, you can simply add w=1 after URL or click here to hide whitespace changes

RoboMWM commented 5 years ago

Even with whitespace changes excluded the diff page is taking way too long to load and still won't load list of files changed.

Either way, it's good practice to PR one feature/update at a time instead of everything all at once and naming it "update" or "upgrade."

jyhsu2000 commented 5 years ago

Thank you for pointing out my fault. Sorry about that. I know it's a bad practice. I was seldom doing that. But this time, I have my reason for doing this. Please ignore the following if you don't want to understand my reason or thinks it's unreasonable.

According to my observation, the author is inactivity for a period of time either on GitHub or SpigotMC. Pull Request cannot be sent based on a commit in another unmerged PR. (or the new PR need to include all the commit in the base PR) And many detail fix/change need to be based on the work of major version changing. If I do them after the first PR (for only compatibility but nothing else) After I sending the first PR, I cannot make sure the following commit are useful and in the right way because the author may make some change after accepting the first PR. Even if I just waiting and do nothing after the first PR sent, I cannot expect how long will we finish the version for 1.13.2. I put the focus on efficiently upgrading this plugin for my server. So I decided to send all the trivial changes with the huge change about compatibility instead of split it into many PR.

BTW, you can click Load diff if it doesn't be loaded automatically as I know.

RoboMWM commented 5 years ago

Oh it loads automatically alright as I watch my scrollbar shrink into oblivion :p

PRs can be automatically merged with base branch after a PR is pulled. I understand it's a bit harder because it's easier to just do all changes in one go on one branch, but if I were the repo owner, there's no way I'd blindly accept thousands of changes to my code all at once.

Perhaps you can squash some of the commits on your branch to break up the diffs, so to speak, and to provide a better summary of each squashed commit's changes.

echurchill commented 5 years ago

I was hoping to get a 1.13.x release out during the holidays (I have been poking at the update for a while).

As for this PR, WOW, this is a lot of changes. I have to admit that the shear number of changes is pretty daunting. Let me look them over.

echurchill commented 5 years ago

the VAST majority of the changes seem to be indent changes. It is making exploring the merge very difficult. Sigh... I will attempt again tomorrow.

echurchill commented 5 years ago

It seems that you are using three spaces for the first intent but four for the rest of them. Am I reading this right?

echurchill commented 5 years ago

Wait, I see now... did you replace the tabs with spaces?

RoboMWM commented 5 years ago

Yea, IMO he should just try to redo his changes but without all of the whitespace and etc. going on. There's a way to view changes without whitespace modifications but even then there are still too many.

I personally would reject such a giant PR, and then cherry pick fixes as I go instead, unless you're okay working with a significant rewrite of your plugin. Up to you. I'd say more stuff but I'd just be repeating myself :p

RoboMWM commented 5 years ago

Also, he or you would have to fix the conflicts before this can be merged now anyways.

jyhsu2000 commented 5 years ago

At first I reformat the code with 4-space indent, but finally I redo it with tab indent.

I can try to resolve conflict first.

Or do I need to redo most of them from the latest commit? I noticed some new commits from @echurchill. But I need some days if we choose this way.

RoboMWM commented 5 years ago

If it's gonna take you a couple days to resolve merge conflicts, IMO that time would probably be better spent re-PRing your changes.

jyhsu2000 commented 5 years ago

I think so. However, I only spent about an hour resolving the conflict. But it'll really take time if I redo another PR. So I asked the question above.

echurchill commented 5 years ago

Any help you can provide would be appreciated. I am happy to look at incorporating your changes, I just want to make sure I am not going to miss anything due to noise. Thanks in advance

echurchill commented 5 years ago

I have been wondering through your changes and I have to say you have been very busy. I really hope we can figure out this merge, it looks like you have fixed a number of things. Good stuff! :-)

jyhsu2000 commented 5 years ago

I'm trying to redo all the things based on 70fabd4d6f65e695ff0f955040f8698d11675ffa. Please wait. After done, I'll send another PR to make commits more clearly and give some of my suggestion.