Skip to content

3 into 1#3

Open
RH-Adv1c3 wants to merge 3 commits intoWaterArchery:masterfrom
RH-Adv1c3:master
Open

3 into 1#3
RH-Adv1c3 wants to merge 3 commits intoWaterArchery:masterfrom
RH-Adv1c3:master

Conversation

@RH-Adv1c3
Copy link
Contributor

1st PR:
Feature Suggestion:

  • Possibility to enable / disable payment rounding
  • Possibility to change the specific integers of said rounding
  • Config options for both to allow the server owner the most flexibility
  • Commented out a boolean that wasn't used in SellTask
  • First attempt at trying to pre-index the common variables so they don't get setup each function call.

2nd PR:
Bug Fix:

  • BuyChest message shows long float number instead of formatted 2 decimal rounded number. Dynamic configurable formatting for ChestShop is WIP.

3rd PR:

  • Changed the WIP to implementation. Uses the same rounding logic as in SellTask and the same config options.

- Possibility to enable / disable payment rounding
- Possibility to change the specific integers of said rounding
- Config options for both to allow the server owner the most flexibility
- Commented out a boolean that wasn't used in SellTask
- First attempt at trying to pre-index the common variables so they don't get setup each function call.
- BuyChest message shows long float number instead of formatted 2 decimal rounded number. Dynamic configurable formatting for ChestShop is WIP.
- Changed the static 2 decimal rounding in ChestShop to dynamic rounding same as in SellTask. Both use the same Config options to not cluster the config.
@RH-Adv1c3
Copy link
Contributor Author

To all developers, yes I know I could've used a helper class to implement the rounding up in a more fashioned way. But currently it would be a little bit overkill and for 2 implementations, a little bit unnecessary.

Copy link
Owner

@WaterArchery WaterArchery left a comment

Choose a reason for hiding this comment

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

Please don't remove existing features like isSellWithLore and notifyOnUnsellable. NotSellingNot and notifyOnUnsellable seems identical.

Also please use camel case rule while naming variables.

Copy link
Owner

@WaterArchery WaterArchery left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Owner

@WaterArchery WaterArchery left a comment

Choose a reason for hiding this comment

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

Thank you!

@WaterArchery
Copy link
Owner

#3 (review)

Thank you for your contrubution. Please fix these.

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.

2 participants