DM-35400: Serve AuxTel movies from RubinTV #82
Conversation
mfisherlevine
force-pushed
the
tickets/DM-35400
branch
from
80ee2db
to
f7ecdbd
Compare
| ) | ||
|
|
||
| the_date = events[0].date |
There was a problem hiding this comment.
It bugs me a little that you need two versions of the date especially since you turn it into a string anyway in the second line of the function on line 315.
My preference would be to only pass around the cleaned string or the object version and deal with manipulation of the thing in the function
There was a problem hiding this comment.
@ugyballoons I'll let you deal with this one.
There was a problem hiding this comment.
I take your point. Date passing across the app is unnecessarily tangled. I think there are 5 representations of any single date in use across model, controller and view. There are two versions of date strings and there should only be one.
| per_day = {} | ||
| movie = get_movie_url(bucket, camera, the_date, logger) | ||
| if movie: | ||
| per_day["movie"] = movie |
There was a problem hiding this comment.
Do you need to initialize this so you don't get a key error if the function doesn't return a truthy thing?
There was a problem hiding this comment.
Looks like get_movie_url() returns either an empty string (unless the request gets a 200) or the URL, so I think this should be fine.
There was a problem hiding this comment.
Then I would leave out the check and let an exception raise. This really looks like the check is trying to do something
There was a problem hiding this comment.
As discussed OOB, the check is (I think, @ugyballoons please chime in here) trying to make sure that they key is only in the dict in the case that the movie URL points to an existing movie.
However, as also discussed OOB @ugyballoons, Simon doesn't think that this model of using the presence/absence of keys in the dict as control flow over whether to display the movie icon is a good plan (assuming that I'm right that that's what's being done). So maybe do comment on this as to how this is intended to work, and if I'm right about how that is, we can come up with a more robust plan here.
There was a problem hiding this comment.
OK. Fair enough. There is no need to re-engineer the entire internals for this, but it would be worth thinking about in any upcoming rewrite. As it stands, it is pretty confusing to read.
There was a problem hiding this comment.
Ah, OK, thanks. In that case, Guy, if I'm right, could you just add a code comment about the presence of that key being control for generating items on the page later, and make a note to self for future redesign perhaps? And if I've got it wrong, just clarify what's happening here for us both?
There was a problem hiding this comment.
(also, side note to Guy - if you don't know about the new "walrus operator", this is a perfect example to write:
if movie := get_movie_url(bucket, camera, the_date, logger)
per_day["movie"] = movie
🙂)
There was a problem hiding this comment.
& hello Simon! I don't think I said hello yet.
I didn't know about the the walrus. Looks good!
The intention behind the dict is as you suspect Merlin, if it's empty, the view can just iterate through and display nothing if there's nothing there. The idea was to keep as much logic in the controller as poss. What's a more satisfactory way of dealing with this?
There was a problem hiding this comment.
Hello! Thanks @ugyballoons for all the work on this!
I don't have the whole thing in my head, so I don't think I can suggest a comprehensive solution. My main point here is that having a dictionary that may or may not have a key set could lead to a confusing exception if some part of that system assumes that key will be defined and it's not. The other option is to always define the key and render based on the value associated with the key. In that case, you could even use a dataclass to define the model up front so everyone know what the interface is.
…ring passing issue
No description provided.