Rename time_resolution_minutes to sample_period_minutes to make the code consistent #587
Conversation
| @@ -98,7 +98,7 @@ def __post_init__(self): # noqa | |||
| @property | |||
| def sample_period_minutes(self) -> int: | |||
| """Override the default sample minutes""" | |||
| return self.time_resolution_minutes | |||
| return self.sample_period_minutes | |||
There was a problem hiding this comment.
Ah, sorry, I hadn't thought about this issue when I wrote issue #587! I don't think Python allows classes where a @property method has the same name as an attribute of the class. (Because, in Python, methods are attributes, too 🙂). Sorry, I should've thought about this!
Perhaps the cleanest solution is to completely get rid of the sample_period_minutes @property method from DataSource and all subclasses of DataSource. But then I think we bump into the issue that dataclass subclasses can't set new default values. (Although maybe that's changed in more recent versions of Python?)
If you're up for it, it'd be amazing if you could have a tinker and see if it's possible to remove the sample_period_minutes @property method, and then find a way to allow subclasses of DataSource to set their own default values for sample_period_minutes?
There was a problem hiding this comment.
@JackKelly I have to learn python for this, I don't know much about python
There was a problem hiding this comment.
OK, no rush! It's totally up to you: If you fancy doing this then please go for it, there's no rush for this issue! But, if you'd prefer not to, then please just shout!
The relevant part of the Python docs might be this page on inheritance in dataclasses. On skim-reading the docs, it actually looks like it might be fine to just delete the sample_period_minutes @property method from all our code, and just use a sample_period_minutes attribute, which can be set to different values for each subclass.
There was a problem hiding this comment.
@JackKelly Sorry for this , I thought it was all about renaming thats why I raised the PR , maybe I am not suitable for this now to do the changes , If you want i will close this pull request and maybe you can assign someone else, i looked at the problem , removing sample_period_minutes @Property method is easy as you know but using just as atribute , I don't know much of it since i am totally unaware of python.
There was a problem hiding this comment.
Cool, absolutely no worries! Don't worry about deleting this PR, as your changes are definitely useful! Maybe someone else can pick up where this left off... Thanks again for your help, and please do keep an eye out for other "renaming" issues!
There was a problem hiding this comment.
@JackKelly If it is useful , can you merge it , maybe some other issue should be opened for adding as attribute , Just a suggestion , Thanks :) also I will start learning python in my free time , thanks for suggestions
There was a problem hiding this comment.
I'm not certain but I think the code might break if we merge as is (even though the unittests pass... but I think the passing tests might be a "false pass" because maybe we don't have any unittests which call the sample_period_minutes @property?)
There was a problem hiding this comment.
@JackKelly yeah got your point , lets wait then for someone to take this
|
@all-contributors please add @RishiKumarRay for code |
|
I've put up a pull request to add @RishiKumarRay! 🎉 |
Pull Request
Description
Rename time_resolution_minutes to sample_period_minutes to make the code consistent
Fixes #584
Checklist: