search

Civcraft / JukeAlert

Do not open issues here; open them on the maintained fork @ DevotedMC
https://github.com/DevotedMC/JukeAlert
BSD 3-Clause "New" or "Revised" License
5 stars 15 forks source link

Add location data to Minecart break results #73

Closed waffle-stomper closed 7 years ago

waffle-stomper commented 7 years ago

Closes #60

I tried to sign the Contributor License Agreement, but I get a "We're sorry, but something went wrong" error.

CivcraftBot commented 7 years ago

Can one of the admins verify this patch? Type 'ok to test' to test.

Maxopoly commented 7 years ago

Yeah, I think the service we previously used for the CLA no longer exists. Did you test this locally?

Maxopoly commented 7 years ago

Change looks good overall, but this would create a weird edge case, where playernames wouldn't show up properly if the player had the same name as any material. Additionally to checking whether the material is not null, could you also check that the action does not equal KILL?

waffle-stomper commented 7 years ago

this would create a weird edge case, where playernames wouldn't show up properly if the player had the same name as any material

Oh wow, I can't believe I missed that. Thanks! I've added the extra check to fix it.

Did you test this locally?

I did. I've also tested the new version with a player named 'MINECART' and everything seemed to work as it's supposed to.

Maxopoly commented 7 years ago

Ok nice, thanks for fixing.

waffle-stomper commented 7 years ago

Are we using the ordinal/ material ID here instead of the victim longform name to save space?

I did that to maintain compatibility with SendSnitchInfo.

Like blocks, is this ID added to the list of IDs at the top of the info display?

Yep!

Maxopoly commented 7 years ago

Ah okay, nice

ProgrammerDan commented 7 years ago

omg, I just had a microstroke.

Great work, been trying to trace that out.

(replace with more mature remark).

Just a note; parsing values out from a string that the application itself generates is very bad form and indicates a problem upstream (better to return an object that holds the value you're needing, then to waste a ton of compute cycles w/ regex pulling them back out of a generated string).

I know you didn't write this, but I'll go ahead and make an issue for someone to come along and fix the structural problems here in the future :D.

In the meantime, good work.

waffle-stomper commented 7 years ago

Thanks for the advice! I have very little formal computer science training so I appreciate any tips.

Once I'm more confident, I might have a go at rewriting that section to remove the string parsing.

ProgrammerDan commented 7 years ago

That'd be awesome! It's a bit more complex of a change but it'd be a great way to "get in deep" so to speak.