💄♻️ Code refactor to support multiple parser #380

mraniki · 2024-07-03T08:06:02Z

Summary by Sourcery

This pull request refactors the code to support multiple parsers by moving configuration settings to instance variables and adding a new handler file. This change enhances the modularity and maintainability of the codebase.

Enhancements:
- Refactored the main parser logic to use instance variables instead of directly accessing settings, improving modularity and maintainability.

sourcery-ai · 2024-07-03T08:06:08Z

Reviewer's Guide by Sourcery

This pull request refactors the code to support multiple parsers by moving configuration settings to instance variables and updating method signatures and logic accordingly. Additionally, a new handler file (findmyorder/handler/simple.py) has been added with a commented-out implementation of the identify_order method.

File-Level Changes

Files	Changes
`findmyorder/main.py` `findmyorder/handler/simple.py`	Refactored the main order parsing logic to support multiple parsers and added a new handler file with a commented-out method for identifying orders.

Tips

Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
Continue your discussion with Sourcery by replying directly to review comments.
You can change your review settings at any time by accessing your dashboard:
- Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
- Change the review language;
You can always contact us if you have any questions or feedback.

sourcery-ai

Hey @mraniki - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 5 issues found
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

sourcery-ai · 2024-07-03T08:06:56Z

findmyorder/main.py

+        self.enabled = settings.findmyorder_enabled
+        if not self.enabled:
+            return
+        self.handler = settings.handler or None


suggestion (bug_risk): Consider initializing self.handler more explicitly.

Using settings.handler or None might mask potential issues if settings.handler is not set correctly. Consider explicitly checking and initializing self.handler to avoid unexpected behavior.

Suggested change

self.handler = settings.handler or None

if hasattr(settings, 'handler'):

self.handler = settings.handler

else:

self.handler = None

sourcery-ai · 2024-07-03T08:06:56Z

findmyorder/main.py

+        self.stop_loss = settings.stop_loss
+        self.take_profit = settings.take_profit
+        self.quantity = settings.quantity
+        self.instrument_mapping = settings.instrument_mapping


suggestion (bug_risk): Consider deep copying mutable settings.

If settings.instrument_mapping is a mutable object, consider using a deep copy to avoid unintended side effects from shared references.

Suggested change

self.instrument_mapping = settings.instrument_mapping

import copy

self.instrument_mapping = copy.deepcopy(settings.instrument_mapping)

sourcery-ai · 2024-07-03T08:06:56Z

findmyorder/main.py

+        self.quantity = settings.quantity
+        self.instrument_mapping = settings.instrument_mapping
+        self.mapping = settings.mapping
+        self.ignore_instrument = settings.ignore_instrument


suggestion (bug_risk): Consider validating settings.ignore_instrument.

Ensure that settings.ignore_instrument is validated to be a list or set to avoid runtime errors.

Suggested change

self.ignore_instrument = settings.ignore_instrument

if not isinstance(settings.ignore_instrument, (list, set)):

raise ValueError("ignore_instrument must be a list or set")

self.ignore_instrument = settings.ignore_instrument

sourcery-ai · 2024-07-03T08:06:56Z

findmyorder/main.py

+        if message:
+            order_identifier = message.split()[0].lower()
+            if order_identifier in (
+                action.lower() for action in self.action_identifiers


suggestion (performance): Consider using a set for self.action_identifiers.

Using a set for self.action_identifiers can improve the performance of the membership test, especially if the list is large.

Suggested change

action.lower() for action in self.action_identifiers

self.action_identifiers = set(settings.action_identifiers)

async def search(self, message: str) -> bool:

if message:

order_identifier = message.split()[0].lower()

if order_identifier in self.action_identifiers:

sourcery-ai · 2024-07-03T08:06:56Z

findmyorder/handler/simple.py

+    # async def identify_order(
+    #     self,
+    #     my_string: str,
+    # ) -> dict:
+    #     """
+    #     Identify an order and return a dictionary
+    #     with the order parameters
+
+    #     Args:
+    #         my_string (str): Message
+
+    #     Returns:
+    #         dict
+
+    #     """
+    #     try:
+    #         action = (
+    #             one_of(self.action_identifier, caseless=True)
+    #             .set_results_name("action")
+    #             .set_parse_action(pyparsing_common.upcase_tokens)
+    #         )
+    #         instrument = Word(alphas + nums).set_results_name("instrument")
+    #         stop_loss = Combine(
+    #             Suppress(self.stop_loss_identifier) + Word(nums)
+    #         ).set_results_name("stop_loss")
+    #         take_profit = Combine(
+    #             Suppress(self.take_profit_identifier) + Word(nums)
+    #         ).set_results_name("take_profit")
+    #         quantity = Combine(
+    #             Suppress(self.quantity_identifier)
+    #             + Word(nums)
+    #             + Optional(Suppress("%"))
+    #         ).set_results_name("quantity")
+    #         order_type = one_of(
+    #             self.order_type_identifier, caseless=True
+    #         ).set_results_name("order_type")
+    #         leverage_type = one_of(
+    #             self.leverage_type_identifier, caseless=True
+    #         ).set_results_name("leverage_type")
+    #         comment = Combine(
+    #             Suppress(self.comment_identifier) + Word(alphas)
+    #         ).set_results_name("comment")
+
+    #         order_grammar = (
+    #             action("action")
+    #             + Optional(instrument, default=None)
+    #             + Optional(stop_loss, default=self.stop_loss)
+    #             + Optional(take_profit, default=self.take_profit)
+    #             + Optional(quantity, default=self.quantity)
+    #             + Optional(order_type, default=None)
+    #             + Optional(leverage_type, default=None)
+    #             + Optional(comment, default=None)
+    #         )
+
+    #         order = order_grammar.parse_string(instring=my_string, parse_all=False)
+    #         logger.debug("Order parsed {}", order)
+    #         return order.asDict()
+
+    #     except Exception as error:
+    #         logger.error(error)
+    #         return error


suggestion: Remove commented-out code.

Consider removing the commented-out code to keep the codebase clean and maintainable. If this code is needed for future reference, it might be better to document it elsewhere.

Suggested change

# async def identify_order(

# self,

# my_string: str,

# ) -> dict:

# """

# Identify an order and return a dictionary

# with the order parameters

# Args:

# my_string (str): Message

# Returns:

# dict

# """

# try:

# action = (

# one_of(self.action_identifier, caseless=True)

# .set_results_name("action")

# .set_parse_action(pyparsing_common.upcase_tokens)

# )

# instrument = Word(alphas + nums).set_results_name("instrument")

# stop_loss = Combine(

# Suppress(self.stop_loss_identifier) + Word(nums)

# ).set_results_name("stop_loss")

# take_profit = Combine(

# Suppress(self.take_profit_identifier) + Word(nums)

# ).set_results_name("take_profit")

# quantity = Combine(

# Suppress(self.quantity_identifier)

# + Word(nums)

# + Optional(Suppress("%"))

# ).set_results_name("quantity")

# order_type = one_of(

# self.order_type_identifier, caseless=True

# ).set_results_name("order_type")

# leverage_type = one_of(

# self.leverage_type_identifier, caseless=True

# ).set_results_name("leverage_type")

# comment = Combine(

# Suppress(self.comment_identifier) + Word(alphas)

# ).set_results_name("comment")

# order_grammar = (

# action("action")

# + Optional(instrument, default=None)

# + Optional(stop_loss, default=self.stop_loss)

# + Optional(take_profit, default=self.take_profit)

# + Optional(quantity, default=self.quantity)

# + Optional(order_type, default=None)

# + Optional(leverage_type, default=None)

# + Optional(comment, default=None)

# )

# order = order_grammar.parse_string(instring=my_string, parse_all=False)

# logger.debug("Order parsed {}", order)

# return order.asDict()

# except Exception as error:

# logger.error(error)

# return error

# Remove the commented-out code to keep the codebase clean and maintainable.

codecov · 2024-07-05T12:42:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (78f3155) to head (fa6a93a).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #380   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         6    +4     
  Lines           59       137   +78     
=========================================
+ Hits            59       137   +78

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

💄♻️ Code refactor to support multiple parser

56754f7

sourcery-ai bot reviewed Jul 3, 2024

View reviewed changes

mraniki added 11 commits July 3, 2024 12:44

💥 breaking with handler template capability

a68df45

⬆️

6372c5a

🎨

0c0cdb6

✅ Unit Test

9ee5519

✅ Unit Test

604c3a1

✅ Unit Test ♻️

5b770f3

🔧

41515cf

♻️

8821d78

🐛

0b268b2

♻️

c3d5eae

✅ Unit Test

f8fea47

mraniki and others added 15 commits July 5, 2024 16:44

✅ Unit Test

beb6220

✅ Unit Test

6839795

✅ Unit Test

2a13706

✅ Unit Test

99f992b

⚡

d6ba261

✅ Unit Test

9e70a50

✅ Unit Test

dc38e54

🐛

3eafa96

test: update test_unit_format_basic.py

56113bf

update basic.py

215d63c

test: update test_unit_format_basic.py

480a45a

test: update test_unit_format_basic.py

2dab3cf

Fix assertion checks for order action and timestamp type.

d38285d

✅ Unit Test

c2a068b

✅ Unit Test

b654144

mraniki added 8 commits July 7, 2024 07:18

✅ Unit Test

7d03370

✅ Unit Test

59d78d2

✅ Unit Test

e8856be

✅ Unit Test

a0228ce

✅ Unit Test

b7e1075

✅ Unit Test

694bdde

✅ Unit Test

aa28581

✅ Unit Test

fa6a93a

mraniki merged commit 8a4b9b0 into main Jul 7, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

💄♻️ Code refactor to support multiple parser #380

💄♻️ Code refactor to support multiple parser #380

mraniki commented Jul 3, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jul 3, 2024 •

edited

Loading

sourcery-ai bot left a comment

sourcery-ai bot Jul 3, 2024

sourcery-ai bot Jul 3, 2024

sourcery-ai bot Jul 3, 2024

sourcery-ai bot Jul 3, 2024

sourcery-ai bot Jul 3, 2024

codecov bot commented Jul 5, 2024 •

edited

Loading

-        self.instrument_mapping = settings.instrument_mapping
+import copy
+self.instrument_mapping = copy.deepcopy(settings.instrument_mapping)

💄♻️ Code refactor to support multiple parser #380

💄♻️ Code refactor to support multiple parser #380

Conversation

mraniki commented Jul 3, 2024 • edited by sourcery-ai bot Loading

Summary by Sourcery

sourcery-ai bot commented Jul 3, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Jul 3, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jul 3, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jul 3, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jul 3, 2024

Choose a reason for hiding this comment

sourcery-ai bot Jul 3, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

mraniki commented Jul 3, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading