Skip to content

add initial support for hex#8

Merged
mrfloris merged 3 commits intomasterfrom
hex-support
May 24, 2021
Merged

add initial support for hex#8
mrfloris merged 3 commits intomasterfrom
hex-support

Conversation

@xsmeths
Copy link
Copy Markdown
Collaborator

@xsmeths xsmeths commented May 24, 2021

simply added my implementation of hex from MedCraft to this

Copy link
Copy Markdown
Contributor

@456dev 456dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a review, but @mrfloris still needs to look over it first :)

overall, looks good

Comment thread src/main/java/com/mrfloris/boosters/pluginEvents.java Outdated
Comment thread src/main/java/com/mrfloris/boosters/pluginEvents.java Outdated
Comment thread src/main/java/com/mrfloris/boosters/pluginEvents.java
Comment thread src/main/java/com/mrfloris/boosters/pluginEvents.java Outdated
Comment thread src/main/java/com/mrfloris/boosters/pluginEvents.java
@mrfloris
Copy link
Copy Markdown
Member

Looks fine so far, i think this is a nice addition to have, and maybe it's important even if we focus on java16/ 1.16.5 and up, that we start adding support for hex. Thank you smeths.

smeths, were you able to test this as well, or was it only code without an in-game test etc?

@456dev

This comment has been minimized.

@mrfloris

This comment has been minimized.

@456dev

This comment has been minimized.

@mrfloris
Copy link
Copy Markdown
Member

Also, if you make a branch, in the future can you prefix it with your GH name
Or create and submit the pull request through a fork

I don't know how to do these things, I click buttons in intellij (base fork, pr), but .. when you make a PR it says who made it etc. and if it isn't master, it should be pretty clear i think? I never bothered to read any docs on this.

It's more for general neatness, as branches are for more than just prs. If you are happying commuting in master for your general house keeping, that fine too. It's more of you do create a branch, how to name it

I understand. ok, for me a PR is just 'look at this, before it goes to master', and can then be deleted. Unless there's a reason for it, for example we have 2 jars, like 1 for legacy servers with & color and 1 for hex (the master, just for 1.16.5 and up) and we maintain both. But at that point the github name of the person submitting wouldn't make sense.

Anyway, it's off topic, when I have time ill merge, test, and cry in my sleep that we got this far with this plugin.

@xsmeths
Copy link
Copy Markdown
Collaborator Author

xsmeths commented May 24, 2021

Looks fine so far, i think this is a nice addition to have, and maybe it's important even if we focus on java16/ 1.16.5 and up, that we start adding support for hex. Thank you smeths.

smeths, were you able to test this as well, or was it only code without an in-game test etc?

yes, it works

@xsmeths
Copy link
Copy Markdown
Collaborator Author

xsmeths commented May 24, 2021

Also, if you make a branch, in the future can you prefix it with your GH name
Or create and submit the pull request through a fork

will bare this in mind
Edit: but what if I don't play Guitar Hero?

Comment thread src/main/java/com/mrfloris/boosters/pluginEvents.java Outdated
Comment thread src/main/java/com/mrfloris/boosters/pluginEvents.java Outdated
@456dev
Copy link
Copy Markdown
Contributor

456dev commented May 24, 2021

i dont think im doing this right, but oh well

@mrfloris
Copy link
Copy Markdown
Member

i dont think im doing this right, but oh well

Maybe something we can look into later, once we can confirm hex codes work in master?

@xsmeths
Copy link
Copy Markdown
Collaborator Author

xsmeths commented May 24, 2021

if statement can be removed for your use-case but I'm not sure the braces are needed or that it will work with them but if it does, feel free to add them

not sure about bracing so I didn't add it as i know it works this way.
@xsmeths xsmeths requested a review from 456dev May 24, 2021 16:27
eh fine, will look at this later

Co-authored-by: the456gamer <The456gamer@the456gamer.dev>
@mrfloris mrfloris merged commit 766e7db into master May 24, 2021
@mrfloris mrfloris deleted the hex-support branch May 24, 2021 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants