Skip to content
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

Implement function convertLineEndingsToNative in blob.ts #2695

Merged
merged 6 commits into from Aug 1, 2019

Conversation

@7k8m
Copy link
Contributor

commented Jul 28, 2019

Implemented the function of convertLineEndingsToNative in blob.ts based on next URL.
https://w3c.github.io/FileAPI/#convert-line-endings-to-native

@qti3e
Copy link
Contributor

left a comment

It generally looks good, it doesn't seem to be any logical issue in the implementation based on the spec, I just left a few suggestions, especially about collectSequenceNotCRLF.

js/blob.ts Outdated
let token = collectionResult.collected;
position = collectionResult.newPosition;

result = result + token;

This comment has been minimized.

Copy link
@qti3e

qti3e Jul 31, 2019

Contributor

result is an empty string, no need to use it.

Suggested change
result = result + token;
result = token;
js/blob.ts Outdated
token = collectionResult.collected;
position = collectionResult.newPosition;

result = result + token;

This comment has been minimized.

Copy link
@qti3e

qti3e Jul 31, 2019

Contributor
Suggested change
result = result + token;
result += token;
js/blob.ts Outdated
}
} else if (c == "\n") {
position++;
result = result + nativeLineEnd;

This comment has been minimized.

Copy link
@qti3e

qti3e Jul 31, 2019

Contributor
Suggested change
result = result + nativeLineEnd;
result += nativeLineEnd;
js/blob.ts Outdated
while (position < s.length) {
let c = s.charAt(position);
if (c == "\r") {
result = result + nativeLineEnd;

This comment has been minimized.

Copy link
@qti3e

qti3e Jul 31, 2019

Contributor
Suggested change
result = result + nativeLineEnd;
result += nativeLineEnd;
js/blob.ts Outdated
// TODO(qti3e) Implement convertLineEndingsToNative.
// https://w3c.github.io/FileAPI/#convert-line-endings-to-native
return s;
let nativeLineEnd = "\n";

This comment has been minimized.

Copy link
@qti3e

qti3e Jul 31, 2019

Contributor
Suggested change
let nativeLineEnd = "\n";
let nativeLineEnd = build.os == "win" ? "\r\n" : "\n";
js/blob.ts Outdated
if (c == "\r" || c == "\n") {
break;
}
result = result.concat(c);

This comment has been minimized.

Copy link
@qti3e

qti3e Jul 31, 2019

Contributor

There is no need to do this call on every iteration, you can obtain the same result using slice() at the end.

const start = position;
while (...) { ... }
return { collected: s.slice(start, position), newPosition: position };

I would probably use something like this:

function collectSequenceNotCRLF(s, position) {
  const start = position;

  for (
    let c = s.charAt(position);
    position < s.length && !(c == '\r' || c == '\n');
    c = s.charAt(++position)
  );

  return { newPosition: position, collected: s.slice(start, position) };
}
@7k8m

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

👌

@ry

ry approved these changes Aug 1, 2019

Copy link
Collaborator

left a comment

LGTM - thanks!

@ry ry merged commit deec1b9 into denoland:master Aug 1, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@7k8m

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@piscisaureus piscisaureus referenced this pull request Aug 9, 2019

Merged

v0.14.0 #2754

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.