Skip to content
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

Bits Helper #939

Merged
merged 21 commits into from
Dec 27, 2024
Merged

Bits Helper #939

merged 21 commits into from
Dec 27, 2024

Conversation

Fluboxer
Copy link
Contributor

Shows you best coins per bit and highlights them
изображение
Supports showing quick selling items (green), as well as best of all (yellow)
изображение

Took me way longer than it should've taken tbh

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Aug 18, 2024
@AzureAaron AzureAaron added the new feature This issue or PR is a new feature label Aug 18, 2024
@AzureAaron AzureAaron added this to the 1.23.0 milestone Aug 19, 2024
@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes reviews needed This PR needs reviews and removed reviews needed This PR needs reviews changes requested This PR need changes labels Aug 19, 2024
@Fluboxer Fluboxer requested a review from AzureAaron August 25, 2024 18:39
@LifeIsAParadox LifeIsAParadox added merge conflicts This PR has merge conflicts that need solving. and removed reviews needed This PR needs reviews labels Sep 2, 2024
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

The method and field order are a bit confusing. For example addToTooltip -> calculateBestItems -> calculateBestInCategory -> processCategory jumps all over the place. Fields should also be placed first.

@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed merge conflicts This PR has merge conflicts that need solving. labels Sep 8, 2024
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Looks good, but can you rebase this. If you don't I'll rebase it, but it'll say authored by you and committed by me.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Sep 8, 2024
@LifeIsAParadox LifeIsAParadox added merge conflicts This PR has merge conflicts that need solving. reviews needed This PR needs reviews and removed changes requested This PR need changes merge conflicts This PR has merge conflicts that need solving. labels Sep 8, 2024
@kevinthegreat1
Copy link
Collaborator

kevinthegreat1 commented Sep 13, 2024

This is good but it seems like the rebase didn't work. While you're on your bits branch, it should be:

git pull origin
git rebase master
git push --force-with-lease

Again, you can ask me to do it if you have any problems.

@kevinthegreat1
Copy link
Collaborator

Thanks for your contribution, but I think the code quality could be improved. I cleaned up some conventions, and you could look at my commits. I'll look at the algorithm parts later, but it seems a bit overly complicated. Next time when writing a feature or algorithm, I suggest thinking through or designing the algorithm on paper first before jumping into code.

Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

I'll just approve this. This doesn't affect other stuff anyway.

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Dec 27, 2024
@kevinthegreat1 kevinthegreat1 merged commit 4185b2c into SkyblockerMod:master Dec 27, 2024
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This issue or PR is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants