You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Issue #142 resulted in a recording file with :config nil. When loading the Rendering page, having one or more files with a nil config caused a null exception. I'm looking for feedback before I submit a pull request.
A better option IMO is to show any misconfigured recordings, perhaps as a second list on the Rendering page. As I was exploring this, I noticed the following:
:name option is redundant because it is just the name of the file.
if :name is ever changed (without a corresponding filename change), the file will never be linked to on the Rendering page and all kinds of weird loading dependencies can occur (e.g. file 1 loads file 2, file 2 loads file 1)
the recording name is duplicated; it is sent as the url and sent in the form params. That's why issue #142 still allowed the file to be created with the correct name, even though the config data in the form params was lost.
So, suggestions:
Submit recordings to a general url and get the recording name from the form params
Remove :name from the recording file and update code to just rely on the file name
Benefits:
Potentially catch recording data errors earlier (file creation will fail, instead of creating invalid files)
All files will be listed, even files that are invalid
Issue #142 resulted in a recording file with
:config nil
. When loading the Rendering page, having one or more files with a nil config caused a null exception. I'm looking for feedback before I submit a pull request.A simple fix is to just filter out the recordings that don't have a name. The downside is that misconfigured recordings simply won't show up in the list, leaving the user to figure out why.
A better option IMO is to show any misconfigured recordings, perhaps as a second list on the Rendering page. As I was exploring this, I noticed the following:
:name
option is redundant because it is just the name of the file.:name
is ever changed (without a corresponding filename change), the file will never be linked to on the Rendering page and all kinds of weird loading dependencies can occur (e.g. file 1 loads file 2, file 2 loads file 1)So, suggestions:
:name
from the recording file and update code to just rely on the file nameBenefits:
Thoughts before I submit a pull request?
The text was updated successfully, but these errors were encountered: