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

Open
wants to merge 9 commits into
base: master
from

Conversation

4 participants
@bios21
Copy link

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

This comment has been minimized.

Copy link

commented Jul 3, 2019

CLA assistant check
All committers have signed the CLA.

@bios21 bios21 force-pushed the ParisDeno:feature/yaml branch from 419000f to 0e52744 Jul 3, 2019

@zekth

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

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

@bios21

This comment has been minimized.

Copy link
Author

commented Jul 3, 2019

Yes will do

@zekth

This comment has been minimized.

Copy link
Contributor

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

bios21 added some commits Jul 4, 2019

@bios21 bios21 changed the title WIP: :sparkles: Add YAML module Add YAML module Jul 7, 2019

@bios21 bios21 marked this pull request as ready for review Jul 7, 2019

@bios21

This comment has been minimized.

Copy link
Author

commented Jul 7, 2019

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

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

This comment has been minimized.

Copy link
@zekth

zekth Jul 7, 2019

Contributor

why?

This comment has been minimized.

Copy link
@bios21

bios21 Jul 7, 2019

Author

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

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

This comment has been minimized.

Copy link
@zekth

zekth Jul 7, 2019

Contributor

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 ?

This comment has been minimized.

Copy link
@bios21

bios21 Jul 7, 2019

Author

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

This comment has been minimized.

Copy link
@bios21

bios21 Jul 7, 2019

Author

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

@zekth

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

Some comments added. Also summoning @bartlomieju for review.

@bartlomieju
Copy link
Contributor

left a comment

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

Show resolved Hide resolved encoding/yaml/Mark.ts

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

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 7, 2019

Contributor

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.

This comment has been minimized.

Copy link
@bios21

bios21 Jul 7, 2019

Author

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

styleAliases?: ArrayObject;
}

function checkTagFormat(tag: string): string {

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 7, 2019

Contributor

seems superfluous

Show resolved Hide resolved 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
@bios21

This comment has been minimized.

Copy link
Author

commented Jul 7, 2019

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

Show resolved Hide resolved encoding/yaml/devDeps.ts Outdated

bios21 added some commits Jul 7, 2019

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