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

Api calculate #11 #80

Merged
merged 8 commits into from
Nov 21, 2023
Merged

Api calculate #11 #80

merged 8 commits into from
Nov 21, 2023

Conversation

wingechr
Copy link
Collaborator

@wingechr wingechr commented Nov 9, 2023

No description provided.

@wingechr wingechr marked this pull request as ready for review November 21, 2023 11:21
@wingechr wingechr requested a review from joAschauer November 21, 2023 12:05
@wingechr wingechr linked an issue Nov 21, 2023 that may be closed by this pull request
@joAschauer joAschauer changed the title draft: Api calculate 11 (Work in progress) Api calculate 11 Nov 21, 2023
@joAschauer joAschauer changed the title Api calculate 11 Api calculate #11 Nov 21, 2023
Copy link
Collaborator

@joAschauer joAschauer left a comment

Choose a reason for hiding this comment

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

Hi @wingechr, thanks for the work. I clicked around in the app and can confirm that #84 and #107 are resolved. You can merge, if you want, you can address my comments before but not necessarily.

tests/test_api_data.py Outdated Show resolved Hide resolved
Comment on lines 37 to 41
class PtxCalc:
def __init__(
self,
data_handler,
):
def __init__(self, data_handler: DataHandler):
self.data_handler = data_handler

def calculate(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could get rid of the class here and just pass data_handler to a function calculate.

Comment on lines 37 to 41
class PtxCalc:
def __init__(
self,
data_handler,
):
def __init__(self, data_handler: DataHandler):
self.data_handler = data_handler

def calculate(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a long function. Can calculate be refactored to smaller functions? might be worth it if we need to touch it in the future again, otherwise let's keep it this way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes,I want to go back to a class based implementation now that it works

yea, thanks! I am not used to pytest yet.

Co-authored-by: Johannes Aschauer <[email protected]>
@wingechr wingechr merged commit 22e4d9b into develop Nov 21, 2023
3 checks passed
@wingechr wingechr deleted the api-calculate-11 branch November 22, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants