-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DONE] Bump video.js to 7.21.2 (latest-7) #1251
Conversation
+ correct some JSDoc formats + Uniformize default DEBUG value to True + Replace some `b` HTML tag by `strong`
This reverts commit e67b8fe.
function showSpeakerModal(speaker = null) { | ||
// Clear existing job fields | ||
document.getElementById('jobFieldsContainer').textContent = ''; | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dans ce fichier, c'est juste la taille des indentations qui passe de 4 à 2 espaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing critical found, just some comments :
pod/main/rest_router.py
-> Why keep comments (l29,30)
pod/main/rest_views.py
-> Why keep comments (l11-16, 24-28)
pod/settings.py
-> I dont have knolege to validate the SERIALIZER
pod/speaker/static/speaker/js/speakers-management.js
-> I dont have knolege for speaker (Do not use and don't know the functionnality so I just trust the code)
pod/video/models.py
-> Why keep comments (l1277-1284)
pod/video/templatetags/video_tags.py
-> Why keep comments (l8, 11-14, 153-220, 342,343)
pod/video/utils.py
-> I did not check to get_storage_path_video replacement method (so I just trust ;)
pod/video/views.py
-> Why keep comments (l58, 908-911)
Hello Guillaume ! |
J'avais d'abord basé cette PR sur la branche 4.x, une fois rebasée sur la branche 3.x ces 3 fichiers .py ne sont plus modifiés. Désolé du couac temporaire. :/
Dans ce fichier, c'est juste la taille des indentations qui passe de 4 à 2 espaces, je n'ai rien changé d'autre ;)
Pour tous ces .py, c'est pareil, les changements étaient dus au rebase, elles ont disparues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est normal de mettre par défaut DEBUG à True ?
Ce script est appelé par un job CRON, et le fait d'afficher du texte pourrait entraîner potentiellement l'envoi massif de mails, il me semble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'était pour être cohérent avec le reste du code. soit on met DEBUG à True partout soit on le met à False partout, mais la y'a un peu de tout, on sait jamais s'il est True ou False du coup.
Il me semble pourtant que les commandes lancée via "python manage.py etc..." utilisent le settings_local.py, comme tout le reste de Pod. Dans ce cas, il prendra le paramètre "DEBUG" défini, et donc à priori sera bien à FALSE dans un environnement en Prod, non ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui oui, cela va bien utiliser le settings_local.py, donc logiquement le DEBUG est à False en prod. C'était juste pour les établissements qui auraient oublier ce paramètre DEBUG.
Ok, si c'est plus cohérent, banco.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sur le coup, j'ai l'impression que ce DEBUG n'est pas utilisé à ce niveau, donc, pas de soucis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S'il n'est pas utilisé, on peut même le virer alors ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui, je pense aussi (si je ne me trompe pas).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le fait de changer par défaut ne risque pas également d'afficher trop de détails sur la connexion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne change que le défaut, qui est bien indiqué à "True" dans la Doc. Je n'ai rien contre le mettre à False par défaut, mais alors il faut aussi le changer partout ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merci Olivier
Replace videojs 7.20.3 by 7.21.2 (latest-7)
b
HTML tag bystrong