Feature/allow setting resolution level #10
Conversation
SeanLeRoy
requested review from
toloudis,
hughes036,
evamaxfield,
yrkim98,
tyler-foster and
BrianWhitneyAI
Welcome to Codecov 🎉Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment. Thanks for integrating Codecov - We've got you covered ☂️ |
| The current resolution level. | ||
| """ | ||
| return self._current_resolution_level | ||
|
|
There was a problem hiding this comment.
It seems backward to me to initialize a current resolution level and then build resolution levels from that initialization.
There was a problem hiding this comment.
I might be misunderstanding what you are getting at, but the reason the _current_resolution_level property has to exist in conjunction to a property of similar name (current_resolution_level) is because the former is protected from getting set by users outside of the way we want (as in we don't want _current_resolution_level = 3 and do want set_resolution_level(3)) and the latter provides an access point to users. We could do with just the private _current_resolution_level but generally you don't want to build an API expecting users to have to access a private property.
SeanLeRoy
force-pushed
the
feature/allow-setting-resolution-level
branch
from
30a5416 to
5abddb1
Compare
SeanLeRoy
force-pushed
the
feature/allow-setting-resolution-level
branch
from
961a393 to
c2fbc48
Compare
| @@ -194,6 +195,28 @@ def current_scene_index(self) -> int: | |||
| """ | |||
| return self._current_scene_index | |||
|
|
|||
| @property | |||
| def resolution_levels(self) -> Tuple[int, ...]: | |||
There was a problem hiding this comment.
can't we just assume they are contiguous indices starting from zero? So all this would have to do is return a count. I think the premise here is that they are just indexed, which is different than how scenes are handled.
There was a problem hiding this comment.
There's nothing wrong with this I suppose, but it's possibly engineering in more than what is needed.
There was a problem hiding this comment.
My thought was that if we keep it close to how scenes are used the API would be simpler/more consistent
This is an attempt to add some getter/setters around resolution level, the idea is to keep the usage patterns close to how scenes are used.
Should resolve #8