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

Reduce method side effects #66

Closed
mgcth opened this issue Mar 7, 2023 · 5 comments · Fixed by #103
Closed

Reduce method side effects #66

mgcth opened this issue Mar 7, 2023 · 5 comments · Fixed by #103
Assignees
Milestone

Comments

@mgcth
Copy link
Collaborator

mgcth commented Mar 7, 2023

Description

Reduce or eliminate (as much as possible) side effects from our methods. Along these lines, remove the Metobs class altogether and just use the individual components.

@mgcth mgcth added this to the 0.2.0 milestone Mar 7, 2023
@docNord
Copy link
Collaborator

docNord commented Mar 22, 2023

How do you think the structure should be in order to avoid such side effects, and what side effects are you thinking of?

@mgcth
Copy link
Collaborator Author

mgcth commented Mar 22, 2023

We could probably delete this class altogether

class Metobs:

For example, get_periods doesn't actually return anything, but just mutates an internal state periods of the object

def get_periods(
self, station: Optional[int] = None, stationset: Optional[str] = None
):
"""Get SMHI Metobs API (version 1) periods from given stations or stationset.
Args:
station: integer id of station
stationset: exact title of station
"""
if self.stations is None:
logging.info("No stations found, call get_stations first.")
return
if station is None and stationset is None:
raise NotImplementedError("Both arguments None.")
if station:
self.periods = Periods(self.stations, station)
else:
self.periods = Periods(self.stations, stationset=stationset)

There are probably more examples.

I think it makes the code harder to test and reason about, and as a user, I would expect get_... to return something and not only modify the object state.

I'd go for pure functions as much as possible, as long as we don't copy data unecessairly but that shouldn't be a big problem as the data here is relatively small for each request. It should simplify any future multithreading/async additions we'd want to add.

@docNord
Copy link
Collaborator

docNord commented Mar 23, 2023

So... Should we pencil out a plan for how we want to structure things before we start remodeling? Or do you already have a blueprint like this in mind?

@mgcth
Copy link
Collaborator Author

mgcth commented Mar 23, 2023

Maybe we should try to release version 0.1.0 first :)

@docNord
Copy link
Collaborator

docNord commented Mar 23, 2023

Good point :)

@mgcth mgcth self-assigned this Feb 20, 2024
@mgcth mgcth mentioned this issue Mar 24, 2024
11 tasks
@mgcth mgcth closed this as completed in #103 Apr 5, 2024
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 a pull request may close this issue.

2 participants