feat: multipart, etc.. #180

keroxp · 2019-02-08T17:17:50Z

added multipart module that read/write multipart/form-data
added some related modules
formated

about `stringsRerader` and `StringReader`

currently stringsReader() is factory of Buffer
I implemented another version of it without Buffer
It looks fine but bufio_test.ts failed
It seems that BufReader#readBytes always returns -1 if data read has finished by first read
- I don't know whether It is Bufreader's bug or StringReader's bug

…d-multipart

ry · 2019-02-08T17:41:37Z

bytes/bytes_test.ts

+  );
+  assertEqual(i, 2);
+});
+test(function testFindLastIndex() {


please have blank line between test cases.

Don't prefix test cases with "test" but use "bytes" ... EG "bytesFindLastIndex"

OK, but is there prettier rule for that ?

ry · 2019-02-08T17:42:25Z

tsconfig.json

@@ -0,0 +1,9 @@
+{


Don't add this file. Even if it's needed, we need to think thru the details of adding tsconfig.json (which we haven't)

ry · 2019-02-08T17:42:57Z

multipart/multipart_test.ts

+  assertEqual(n, 3);
+  assertEqual(err, "EOF");
+});
+test(function testScanUntilBoundary4() {


Rename this tests and others: multipartScanUntilBoundary4

kitsonk

A few comments

kitsonk · 2019-02-09T11:28:59Z

.gitignore

@@ -0,0 +1,4 @@
+.DS_Store


All of these should simple not be committed, you can add .DS_Store to your global git config and the rest have no legitimate reason to be included.

I disagree. Why don't we have .gitignore? It's actually common.

It's fine with me - .gitignore is cheap

It maybe cheap, but why don't we have these developer unique ignores in Deno then?

I'm fine with that in Deno too shrug

kitsonk · 2019-02-09T11:29:32Z

bytes/bytes.ts

@@ -0,0 +1,54 @@
+export function bytesFindIndex(a: Uint8Array, pat: Uint8Array): number {


There really should be some sort of JSDoc.

kitsonk · 2019-02-09T11:30:40Z

bytes/bytes.ts

+  const s = pat[0];
+  for (let i = 0; i < a.length; i++) {
+    if (a[i] !== s) continue;
+    let pin = i;


pin is not reassigned, should be const

We need tslint in deno_std...

kitsonk · 2019-02-09T11:31:34Z

bytes/bytes.ts

+  const e = pat[pat.length - 1];
+  for (let i = a.length - 1; i >= 0; i--) {
+    if (a[i] !== e) continue;
+    let pin = i;


pin is not reassigned, should be const

kitsonk · 2019-02-09T11:33:16Z

io/readers.ts

+const encoder = new TextEncoder();
+
+export class StringReader implements Reader {
+  private offs = 0;


We should use the _ sigil to preface private members.

We don't have any convention about naming of private members. IMO, _ is redundant if there are no special reason.

Ya... we need to decide one way or another at some point on this. We've got code that does both. Obviously @kitsonk is on the side of _ ... I don't particularly care as long as its uniform ... I think we need a style guide PR and the ensuing ~~blood bath~~ discussion to figure it out : )

I have argued multiple times, there are two special reasons. To which no one has had an argument against.

On TypeScript private members take up a slot on the API surface, because they aren't really private. So you could be squatting on something semantically meaningful in a subclass. While this is likely rare in practice, it is still good form.

When private members do arrive, they will use the # sigil. To use them, we have to refactor at both the declaration and call site. Using _ means easier refactoring down the road.

Plus there is that it has been a common convention for a long long time and it makes the intent at the call site more clear, that you are accessing a private member.

Anyway, those discussion should not be concluded at here. Let's have discussion on another issue.

kitsonk · 2019-02-09T11:34:39Z

io/ioutil.ts

+import { Reader, Writer } from "deno";
+import { assert } from "../testing/mod.ts";
+
+/* copy N size at the most. If read size is lesser than N, then returns nread */


Should be JSDoc /**

kitsonk · 2019-02-09T11:45:07Z

io/util.ts

+    postfix?: string;
+  } = { prefix: "", postfix: "" }
+): Promise<{ file: File; filepath: string }> {
+  const r = ~~(Math.random() * 1000000);


feels a bit strange to me... the double bitwise operator... at the least it is unclear... while I am not rejecting it, it does feel like it temporary files are something that Deno itself should be handling. Especially because there is no contract here to purge it.

@ry it feels like a tempfile is something that runtime users should be abstracted from.

@keroxp I agree that the double ~ needs a comment - I don't know what that's doing.

This feels like a function that can be moved into a common utility module at some point (or even into core) but I think for now let's just land it here.

I believe it gets rid of decimal part of number, ie. ~~1.23456 === 1

I see. I'd prefer if @keroxp uses Math.floor() then as it's more readable.

kitsonk · 2019-02-09T11:45:41Z

io/writers.ts

@@ -0,0 +1,37 @@
+import { Writer } from "deno";
+
+const e = new TextEncoder();


would prefer encoder and decoder, we don't need to be human minifiers.

kitsonk · 2019-02-09T11:46:19Z

io/writers.ts

+const e = new TextEncoder();
+const d = new TextDecoder();
+export class StringWriter implements Writer {
+  private chunks: Uint8Array[] = [];


again, private memers should start with a _ sigil

keroxp · 2019-02-09T18:04:22Z

Added JSDoc comment for newly added resources.
placed shared TextEncoder and TextDecoder instances on strings/strings.ts

ry · 2019-02-09T18:25:38Z

multipart/fixtures/sample.txt

+bar
+----------------------------434049563556637648550474
+content-disposition: form-data; name="file"; filename="tsconfig.json"
+content-type: application/octet-stream


Nice tests!

ry · 2019-02-09T18:26:26Z

strings/strings.ts

+export const encoder = new TextEncoder();
+
+/** Shorthand for new TextEncoder().encode() */
+export function encode(input?: string): Uint8Array {


Sure. I guess this is useful.

Needs tests though.

ry

This is great functionality to have. Just one nit about the double tilde and then we can land.

- Changed return type of `MultipartReader.readForm()` to map of string | FormFile - This is because deno's FormData can only accept string or instance of DenoBlob - Added more tests about MultipartReadre and tempFile

keroxp · 2019-02-10T17:42:56Z

@ry review applied. And there are additional changes about readForm result. This is because:

deno's FormData can't accept field value except string or DenoBlob
DenoBlob is not suitable for form file value
- Blob is designed for immutable, in-memory data

ry · 2019-02-10T23:48:05Z

io/readers.ts

+}
+
+/** Reader utility for combining multiple readers */
+export class MultiReader implements Reader {


Yes! I like it when we copy Go : )

ry

LGTM

In the future please go through can click "Resolve Comment" - it makes it easier to skim comments : )

Original: denoland/std@fda9c98

keroxp added 5 commits February 9, 2019 01:48

feat: multipart

1ca01b9

fix: tests

3fa7def

Merge branch 'master' of https://github.com/denoland/deno_std into ad…

22c262a

…d-multipart

fmt

0c88603

Merge branch 'master' of https://github.com/denoland/deno_std into ad…

dea00f6

…d-multipart

ry reviewed Feb 8, 2019

View reviewed changes

keroxp added 3 commits February 9, 2019 02:45

git: add deno.d.ts, tsconfig.json

568f30a

fix: test naming

204cf76

fix: multipart test file paths

44065fb

kitsonk reviewed Feb 9, 2019

View reviewed changes

add: JSDoc comments

48206a3

fmt

8972d17

ry reviewed Feb 9, 2019

View reviewed changes

keroxp added 2 commits February 11, 2019 02:19

fmt: add copy rights

7828ba0

fix: changes as follows:

728ca84

- Changed return type of `MultipartReader.readForm()` to map of string | FormFile - This is because deno's FormData can only accept string or instance of DenoBlob - Added more tests about MultipartReadre and tempFile

add: tests for MultipartWriter

d9497aa

ry reviewed Feb 10, 2019

View reviewed changes

ry approved these changes Feb 10, 2019

View reviewed changes

ry merged commit fda9c98 into denoland:master Feb 10, 2019

ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019

feat: multipart, etc.. (denoland/std#180)

33f6278

Original: denoland/std@fda9c98

		@@ -0,0 +1,54 @@
		export function bytesFindIndex(a: Uint8Array, pat: Uint8Array): number {

		@@ -0,0 +1,37 @@
		import { Writer } from "deno";

		const e = new TextEncoder();

feat: multipart, etc.. #180

feat: multipart, etc.. #180

Conversation

keroxp commented Feb 8, 2019 • edited Loading

about stringsRerader and StringReader

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keroxp Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keroxp Feb 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keroxp commented Feb 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

keroxp commented Feb 10, 2019

ry Feb 10, 2019 • edited Loading

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

keroxp commented Feb 8, 2019 •

edited

Loading

about `stringsRerader` and `StringReader`

keroxp Feb 9, 2019 •

edited

Loading

keroxp Feb 9, 2019 •

edited

Loading

ry Feb 10, 2019 •

edited

Loading