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

Add YAML module #528

Closed
wants to merge 9 commits into from
Closed

Conversation

lsagetlethias
Copy link
Contributor

@lsagetlethias lsagetlethias commented Jul 3, 2019

This a a port from nodeca/js-yaml.

Typing internally is quite messy but the endpoints themselves (parse and stringify) are ready to use.

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2019

CLA assistant check
All committers have signed the CLA.

@zekth
Copy link
Contributor

zekth commented Jul 3, 2019

Could you move it to encoding/yaml? Like toml and csv?

@lsagetlethias
Copy link
Contributor Author

Yes will do

@zekth
Copy link
Contributor

zekth commented Jul 3, 2019

Also please add references to https://github.com/nodeca/js-yaml in headers of ported files like here : https://github.com/denoland/deno_std/blob/master/encoding/csv.ts#L1

@lsagetlethias lsagetlethias changed the title WIP: ✨ Add YAML module Add YAML module Jul 7, 2019
@lsagetlethias lsagetlethias marked this pull request as ready for review July 7, 2019 18:22
@lsagetlethias
Copy link
Contributor Author

@zekth this PR is now RFR, tell me if squash is needed.

README.md Outdated Show resolved Hide resolved
tsconfig.json Outdated
@@ -1,5 +1,5 @@
{
"extends": "tsconfig.test.json",
"extends": "./tsconfig.test.json",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was a need when I setup my dev environment with yarn / node and executing eslint without npx.
Rollbacked.

encoding/yaml.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,22 @@
// Copyright 2018-2019 the Deno authors. All rights reserved. MIT license.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tests files are not covering all the features of the lib.
Could you port those from here: https://github.com/nodeca/js-yaml/tree/master/test ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS files copied. Will do the conversion in ts/deno asap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will also be included issues specific tests from js-yaml repository.

@zekth
Copy link
Contributor

zekth commented Jul 7, 2019

Some comments added. Also summoning @bartlomieju for review.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good @bios21, tests seem a bit short, can you port more as @zekth suggested?

encoding/yaml/Mark.ts Show resolved Hide resolved

export abstract class State {
constructor(public schema: SchemaDefinition = DEFAULT_FULL_SCHEMA) {}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that these objects are so well organized but... splitting them into 4 files means there are 4 more deps to download. I'd consider putting them into single file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean all 4 kinds of schema to be grouped as one file ?

styleAliases?: ArrayObject;
}

function checkTagFormat(tag: string): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems superfluous

encoding/yaml/error/YAMLError.ts Outdated Show resolved Hide resolved
encoding/yaml/schema/core.ts Outdated Show resolved Hide resolved
encoding/yaml/schema/default_full.ts Outdated Show resolved Hide resolved
encoding/yaml/schema/json.ts Outdated Show resolved Hide resolved
@lsagetlethias
Copy link
Contributor Author

Thank you for your reviews. I'm on it ASAP. 👌

export type DumpOptions = DumperStateOptions;

export function stringify(obj: object, options?: DumpOptions): string {
return dump(obj, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export { dump as stringify };
?

result.push((bits >> 4) & 0xff);
}

return new Buffer(new Uint8Array(result));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does Deno have Buffer ?
why not just return new Uint8Array(result) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants