Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add MultipartHttpClient to handle file upload #7
Add MultipartHttpClient to handle file upload #7
Changes from 1 commit
1dbbe13
6911fa8
873d16b
60da03c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Ce serait pas possible de capitaliser sur https://github.com/Coreoz/simple-http-request-builder/blob/master/src/lib/HttpRequest.ts ?
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.
quand tu dis capitaliser c'est à dire ? réutiliser ou juste plus s'en servir ?
pour moi, faire les deux dans la même classe va complexifier un code se voulant assez simple.
en revanche, peut etre pourrait-on plus l'utiliser avec une API type ReadableStream, mais ca n'est pas le code fait ici
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 suis de l'avis de Lucas sur la séparation. Le code exporté par la lib pouvant être réutilisé l'a été dans le fichier.
Par contre, je pense qu'il faudrait réorganiser les dossiers de la lib car tout est à la racine pour l'instant
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 comprends que ça serait compliqué d'utiliser HttpRequest.ts ici.
Et effectivement, il faudrait déplacer les 2 fichiers dans un dossier dédié
multipart
. De plus, il faudrait documenter le fonctionnement de ces fichiers (comment utiliser le client multipart ? dans quel cas ?) dans le fichierREADME.md
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 fait pour le dossier multipart.
Le readme était déjà là ahah
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.
pourquoi ces ajouts ? pour moi c'est propre à chaque projet
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.
parce que, la plupart du temps, on veut s'en servir pour uploader des fichiers donc autant avoir les méthodes dispos sur la requête directement. Je peux supprimer si c'est superflu