feat: asso edition #87
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #87 +/- ##
==========================================
+ Coverage 83.19% 84.48% +1.29%
==========================================
Files 140 147 +7
Lines 2398 2624 +226
Branches 470 506 +36
==========================================
+ Hits 1995 2217 +222
- Misses 398 402 +4
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlbanSdl
force-pushed
the
feat/asso/edit
branch
from
467dc68 to
9bd72c8
Compare
AlbanSdl
force-pushed
the
feat/asso/edit
branch
from
255c322 to
7b180e8
Compare
|
Tu pourras edit ton message, elle est plus basée sur #73 ^^ |
| }, | ||
| }; | ||
|
|
||
| export class TranslatedTextDto { |
There was a problem hiding this comment.
Ya le nouveau decorator AtLeastOneOf ou quelque chose comme ça, celui que t'appliquais sur une property et que j'ai revu pour l'appliquer sur une classe
Et je pense qu'on peut mettre les dto génériques dans un fichier ? je pense qu'il y en a déjà un (notamment pour les ArrayDto ou je sais plus comment)
There was a problem hiding this comment.
Btw j'y pense, mais c'est dans ma pr, pour le moment tu peux utiliser le tien et inshallah je penserai à modif ça au moment du merge (tfacons yaura des merge conflicts)
There was a problem hiding this comment.
Yop j'ai laissé le commentaire comme ça pour l'instant du coup
There was a problem hiding this comment.
Ta PR sera merge avant la mienne, donc vazy, tu peux mettre ton déco, je mettrai ça sur le mien
| .withFile('file', `test/e2e/media/image/artifacts/image.gif.png`) | ||
| .expectAppError(ERROR_CODE.FILE_INVALID_TYPE, 'image/png, image/jpeg, image/webp, image/avif, image/tiff'); | ||
| }); | ||
| it('but not allow missing files', async () => { |
There was a problem hiding this comment.
"it but", ça fait pas une belle phrase x)
idem pour les 2 au dessus
There was a problem hiding this comment.
bah ça n'écrit jamais it dans les tests, donc je vois pas l'intérêt de faire un full phrase pour rien
There was a problem hiding this comment.
Mais même à comprendre c'est pas évident, j'imagine que tu veux dire que la route "ne doit pas rescale les gifs, les jsp quoi, et ne pas autoriser si ya pas de fichier manquant" ?
There was a problem hiding this comment.
Factuellement je trouve ça ok (même si j'ai pas update le "should create the annal")
There was a problem hiding this comment.
ptet je devrais remplacer les should par "and" cela dit
| const baseToss = Spec.prototype.toss; | ||
|
|
||
| Spec.prototype.created = function () { | ||
| this.expectedStatus = HttpStatus.CREATED; |
There was a problem hiding this comment.
Pas du tout convaincu par ton implémentation, ça enlève pas de verbose et ça simplifie pas grand chose j'ai l'impression, par contre ça complexifie (dans le sens où c'est pas natif). Complexifier c'est pas nécessairement un problème, et là ça va, c'est pas non plus le fakedb, mais faut quand même qu'il y ait un intérêt à mon avis. Le seul intérêt que je vois, c'est que quand tu fais expect...({}, true), c'est plus joli sans le true, ça évite de l'avoir sur une seule ligne pour lui tout seul.
A la limite, je serais plus pour faire en sorte que chaque fonction prenne un paramètre StatusCode en plus.
Je suis pas fan non plus de l'idée, mais ça serait possible de mettre un déco à la classe (je sais plus exactement comment ça marche en TS mais je crois que ça serait possible), qui prend toutes les fonctions "expect..." et qui leur ajoute un paramètre et un expectStatus en préfix. Mais ça me paraît overkill par rapport à ce que ça apporte, ça cache un petit peu ce que ça fait, et ça alourdit beaucoup le typing (qui est déjà ptet un poil trop lourd ouspy. Tant que j'y parle, un jour faudra peut-être essayer de l'alléger un peu à 2-3 endroits).
Par rapport à l'implémentation, 2-3 trucs quand même :
Ajoute expectedStatus dans le .d.ts, et set Spec.prototype.expectedStatus à StatusCodes.OK pour avoir une valeur par défaut. T'auras pas non plus besoin de delete la variable dans expectStatus.
Sinon, j'ai pas testé, mais pour que expectStatus soit call automatiquement, est-ce que c'est pas possible de le mettre dans la fonction toss ? Ça serait bien de mettre un petit message qui dit quelque chose comme "if you did not expect a 200, use function created for example" (enfin mal formulé mais c'est l'idée)
There was a problem hiding this comment.
Ouais ça permet en effet de faire ça, mais je vois pas en quoi c'est moins bien qu'avant. Ça évite d'avoir deux fonctions qui font quasiment la même chose et ça vérifie le status par défaut
Je suis pas convaincu non plus par l'idée du décorateur.
expectedStatus n'a rien à faire dans la public api de notre extension de Spec selon moi. C'est une valeur utilisée en interne donc inutile de la déclarer
Pour passer dans le toss par contre, ça veut dire généraliser la modification du status (genre les fonctions created etc) sur les erreurs etc
Sinon pour le reste du commentaire jsuis d'accord
Permet d'éditer les infos d'une asso. Cette PR contient également l'upload et le traitement d'images.
Cette PR est basée sur #73 mais je l'ai fait pointer vers
devpour générer un coverage (je ne sais pas encore pourquoi mais impossible de le rapport html en local 🤷)