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

Encapsulate Trailing and Stop #43

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

inverse
Copy link
Contributor

@inverse inverse commented Jul 13, 2021

Encapsulate TRAILING and STOP into distinct objects.

Not 100% sure of this one so please correct me where I mis-interpreted things.

algobot/traders/trader.py Outdated Show resolved Hide resolved
algobot/traders/trader.py Outdated Show resolved Hide resolved
algobot/enums.py Outdated Show resolved Hide resolved
algobot/enums.py Outdated Show resolved Hide resolved
TRAILING = 2
STOP = 1

class OrderType:
Copy link
Contributor Author

@inverse inverse Jul 14, 2021

Choose a reason for hiding this comment

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

Still could be improved but it's a step in the right direction. by improvements we could use real enums and get read of the whole from_str but that would require a lot more mapping ;)

Like mapping the UI element to string -> enum value :)

Copy link
Owner

Choose a reason for hiding this comment

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

nice

@@ -73,7 +73,7 @@ def __init__(self, parent: QMainWindow, logger: Logger = None):
}

self.lossTypes = ("Trailing", "Stop")
Copy link
Owner

Choose a reason for hiding this comment

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

we should probably leverage the actual enum values? let's not use integers but actual strings? what do you think? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could work - trying to think of how would be best to handle semantics vs presentation

Copy link
Owner

Choose a reason for hiding this comment

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

Well, as long as the enums stay the same, we can just replace the value.
So TRAILING can change from 1 to "Trailing", then we don't even need a mapper ;p

@@ -343,16 +343,16 @@ def get_take_profit_settings(self, caller) -> dict:
tab = self.get_category_tab(caller)
dictionary = self.takeProfitDict
if dictionary[tab, 'groupBox'].isChecked():
if dictionary[tab, 'takeProfitType'].currentText() == "Trailing":
takeProfitType = TRAILING
if dictionary[tab, 'takeOrderType'].currentText() == "Trailing":
Copy link
Owner

Choose a reason for hiding this comment

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

we should probably use the to_str method()

@@ -246,9 +246,9 @@ def get_stop_loss_strategy_string(self) -> str:
Returns stop loss strategy in string format, instead of integer enum.
:return: Stop loss strategy in string format.
"""
if self.lossStrategy == STOP:
if self.lossStrategy == OrderType.STOP:
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably deprecate this function and add a kwarg for suffix in the to_str method

self.assertEqual(backtester.get_stop_loss(), 3 * (1 + backtester.lossPercentageDecimal))

def test_stop_take_profit(self):
"""
Test backtester take profit logic.
"""
backtester = self.backtester
backtester.takeProfitType = STOP
backtester.takeOrderType = OrderType.STOP
Copy link
Owner

Choose a reason for hiding this comment

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

nice

from algobot.enums import OrderType


class OrderTypeTest(unittest.TestCase):
Copy link
Owner

Choose a reason for hiding this comment

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

let's use pytest? We should soon start rewriting all unit test tests to leverage pytest :p

Copy link
Owner

@ZENALC ZENALC left a comment

Choose a reason for hiding this comment

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

minor comments

@ZENALC ZENALC self-assigned this Jul 16, 2021
@ZENALC ZENALC added the enhancement New feature or request label Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants