Closed lithorus closed 18 hours ago
Now that I'm looking at your proposal materialized, I have some concerns about how it will impact the project in the future.
* Up until now, rqd was independent of pycue, which means the only contract they had in common was the protobuf layer (Loose coupling). With this change, they now share modules, which is good for reusability, but increases the coupling. If we had a great portion of code being reused, maybe the tradeoff would be fair, but so far we're only reusing cuelogging on READ mode. Which brings me to my next concern * On cuelogging we're exposing the WRITE mode to pycue, which doesn't have any use for it. This would be harmless if we didn't have artists that like to program and hack things, I can easily see an user of the API accidentally initializing `CueLogger` in WRITE mode and inadvertently triggering a log rotate (which is part of the `__init__` function).
I'm really sorry I didn't catch this concerns on the design phase, but I do think keeping pycue(opencue) and rqd as separate modules that only communicate via protobuf weights heavier on the tradeoff between reuse and coupling.
No worries. I think I've re-written this a few times already :)
Having opencue as a dependency, was partly the reason I brought up the dependencies allowed in #1441. The plan was that the cuelogging.py
could be overwritten (by the studio) with a custom one to provide both read and write. I'll create a proof-of-concept of this later. (Have added example on how to override it in the description)
I understand the concern (and wasn't 100% happy with it either), but the main point was to have read and write in the same file atleast. Would it be a better idea to have a CueLogReader and a CueLogWriter instead?
Had to move the cuelogging
outside of the opencue
package so the protobuf's doesn't conflict with each other, but have re-added the compiled_proto to rqd
analize_cuebot
I added the analyze_python
(I guess that's the one you meant?)
Edit:
Looks like it needs a bit of modifiations...
Summarize your change. This moves log reading and writing into a single package, to make it easier to support multiple backends in a single place.
LogViewPlugin.LogReader
andrqdlogging.RQDLogger
have been replaced withcuelogging.CueLogReader
andcuelogging.CueLogWriter
rqd.compiled_proto
has also been replaced withopencue.compiled_proto
inrqd
Have also updated tests and documentation.