CoreProtect Support

[StunningLlama]

Added CoreProtect logging support to most Brushes and Performers that support it. Would be useful to many servers with CoreProtect. CoreProtect logging is optional.

[Mumfrey]

Are you serious? Don't you think it would have been an amazing idea to learn java and git before attempting this? The only reason I'm not hitting "close" right now is that I don't want to deny @MikeMatrix the satisfaction.

[Mumfrey]

Ok I realise that's not very constructive so here are the headlines:

• Why the hell would you put every single change in a different commit? This makes it imposible to get an overview of what this PR does.
• Because of your ridiculous commit ethic, you have commits where you make a syntax error and then in the next commit make a one-character fix for the previous commit's error.
• You literally wrap calls to logging methods around calls to methods that you want to log. How is this a good pattern? Why not delegate? Proxy? Anything except the downright stupid method you've chosen.
• Why static? You clearly have seen singleton pattern somewhere but don't understand it, and the model isn't appropriate here anyway.
• You've created a hard dependency on CoreProtect (do you even ClassLoader?), meaning that VS will not work at all if CP is not present. How is is this a good idea? I'm guessing it's all to do with the fact that you clearly have no clue what you are actually doing. Again, go learn java first before attempting something of this magnitude.
• Why in the name of all godly hell would you use one-character variable names...? The VoxelSniper codebase is old and absolutely littered with junk one-character names which the current devs have been patiently trying to refactor for months now. It's bad, awful-looking code and you're just adding to it. If you make new things, make nice things, don't layer crap on crap which the devs then have to refactor along with everything else.
• Why, why, why oh why would you implement this by directly calling a method on your CoreProtect handler thingy? I mean, if you want a kind of before-change/after-change, why not implement that in an abstract way first and then hook the CP functionality on it. The way you've written this is not extensible in the slightest and just adds more brittle code on top of an already brittle codebase.
• That's not everything, but I feel to carry on wouldn't actually make the point any clearer.

There, hopefully that's more illuminating.

[gabizou]

What am I looking at? A few questions (some that are repeated as Mumfrey beat me to them): 1) How does this make VS less prone to breaking? 2) how does VS benefit from making additional calls directly to CoreProtect? 3) Could you have thought of any shittier way to have implemented this? 4) This adds more code maintenance that @MikeMatrix definitely will not want to do, and neither will I.

If you want, rewrite this to a way that VS recognizes some sort of block protection plugin and can handle the block protections in an abstract way.

[Deamon5550]

Another thing for the list of things to learn before attempting this: Maven

Forget all the other issues, this just straight up won't even compile right now.

[MikeMatrix]

I have nothing to add... Thanks for relieving me of the effort to write this @Mumfrey, @gabizou and @Deamon5550.

[StunningLlama]

Forgive me for being rude, but I didn't know people could act like entitled arrogant assholes just because someone else's code execution was done improperly.

On Wed, Aug 6, 2014 at 7:01 AM, Mumfrey notifications@github.com wrote:

Are you serious? Don't you think it would have been an amazing idea to learn java and git before attempting this? The only reason I'm not hitting "close" right now is that I don't want to deny @MikeMatrix https://github.com/MikeMatrix the satisfaction.

— Reply to this email directly or view it on GitHub https://github.com/TVPT/VoxelSniper/pull/256#issuecomment-51320502.

[gabizou]

Forgive me for being rude, but I didn't know people could act like entitled arrogant assholes just because someone else's code execution was done improperly.

Seems like there's a few things you didn't understand from the comments prior:

• Your implementation doesn't compile due to a lack of dependency resolution for anyone else importing this project to compile. A lack of maven know-how, if you will, and you didn't spend time to figure that out for yourself. Yes, we could fix that easily, but that's not what we have PR's for. PR's are for features that are completed, near completion, or just for the sake of being official.
• The following question's answer is no: "Does this even work if CoreProtect is not available?" Why would we add a hard dependency for VS when a majority of it's use is to simply have better block manipulation? Your implementation doesn't even provide checks if CoreProtect is available. As a maintainer, it is a slap in our face when someone suggests such a PR that adds even more code behavior to maintain.
• We are entitled to be arrogant pricks because we not only maintain the project, but we also know how to implement features sanely in a way that won't make it even more of a pain to maintain in the future. Your PR, as I said in the previous point, tries to smear that ideal into a hot pit of fiery coals, to which we'll naturally defend our project by throwing this PR into said fiery pit of coals.
• Writing a PR where the premise of the PR is false is going to get a lot of flack from the reviewers. That goes for any project. Expecting the maintainers to kindly tell you how to rewrite this implementation, or hold your hand on how to make your implementation less bad, is naive.
• Lastly, from the looks of your commits, you're still learning how to program in Java, let alone write Bukkit plugins. Writing a PR at this level of complexity is not where I would start learning to write a plugin. I would have asked someone else to attempt this for you, possibly someone who knows what they're doing.

All in all, you could have gone about this completely differently, primarily by opening an Issue and requesting a feature for creating an abstraction layer of allowing plugins like CoreProtect to hook into VS and cancel block changes. Otherwise, this is a waste of time.

[Mumfrey]

Forgive me for being rude, but I didn't know people could act like entitled arrogant assholes just because someone else's code execution was done improperly.

If it were only "improperly" then maybe it wouldn't have ilicited such a harsh response, but this is really a thoroughly dreadful pull request and the fact that you actually somehow thought that this was appropriate deserves ridicule as far as I'm concerned.

[MikeMatrix]

And to add to this: We even have guidelines for PRs, so not even following one of the few guidelines we have is just straight out ridiculous.

[AlexCMueller]

It's so embarassing looking back on this... I wish I could forget but I just can't, the horridness of the PR and that I contributed will forever haunt me. :(