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 void type #242

Merged
merged 4 commits into from
Oct 24, 2019
Merged

Add void type #242

merged 4 commits into from
Oct 24, 2019

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Oct 20, 2019

Thanks for the great tool!

This adds a VoidType(|Formatter|Parser).

Some background:

I'm looking into microsoft/language-server-protocol#67, basically reconciling the reference implementation types with the (separately maintained) specification. The good news: tsjsg handles the protocol.ts without complaint. Hooray 🎉!

But... I've been running into a fair number of things that aren't getting pulled in to fully describe/validate what's in the markdown spec, and am having to expand my --paths, thereby encountering new issues. I assume it's failing after the first problem, so this is no doubt going to be one of those epic yak shaves.

The very first one is an interesting use of void:

export namespace ConfigurationRequest {
	export const type = new RequestType<ConfigurationParams & PartialResultParams, any[], void, void>('workspace/configuration');
	export type HandlerSignature = RequestHandler<ConfigurationParams, any[], void>;
	export type MiddlewareSignature = (params: ConfigurationParams, token: CancellationToken, next: HandlerSignature) => HandlerResult<any[], void>;
}

which yields

Error: Unknown node "void" (ts.SyntaxKind = 107) at .../vscode-languageserver-node/protocol/src/protocol.configuration.ts(37,75)

This fixes the void problem (hooray)...

Yak shave n+1:

...revealing the next thing:

Error: Unknown type "function"

Onward!

note: i got this when trying to lint:

yarn run v1.15.2
$ eslint '{src,test,factory}/**/*.ts'

Oops! Something went wrong! :(

ESLint: 6.5.1.

ESLint couldn't find the config "@satazor/eslint-config/es5" to extend from. Please check that the name of the config is correct.

The config "@satazor/eslint-config/es5" was referenced from the config file in "/home/weg/Documents/projects/ts-json-schema-generator/envs/lsp-json-schema/lib/node_modules/npm/node_modules/err-code/.eslintrc.json".

hopefully it all works out!

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

This is great. Thank you for the PR. Just one comment.

],
"additionalProperties": false
},
"MyGeneric<number>": {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this number, not void?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.. apparently, because I copy and paste poorly!

Fixing, testing locally!

The test copy pasta raises some interesting questions: perhaps an interesting place to use both mutation and property testing>

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Add a test case that checks the pair of ts and schema file. There is already a file that runs test cases. We don’t pick up just the files.

@bollwyvl
Copy link
Contributor Author

Added generic-void to the tests, looks okay locally.

Also I noticed that coverage isn't getting picked up:

==> Uploading reports
    url: https://codecov.io
    query: branch=pull%2F242&commit=0edc3e90786199baf8c71b284599d4c94ec4eaf7&build=189&build_url=&name=189&tag=&slug=vega%2Fts-json-schema-generator&service=circleci&flags=&pr=242&job=0
    -> Pinging Codecov
https://codecov.io/upload/v4?package=bash-8a28df4&token=secret&branch=pull%2F242&commit=0edc3e90786199baf8c71b284599d4c94ec4eaf7&build=189&build_url=&name=189&tag=&slug=vega%2Fts-json-schema-generator&service=circleci&flags=&pr=242&job=0
HTTP 400
Provided token is not a UUID.

I wonder if that would that have caught it?

@domoritz
Copy link
Member

The coverage would maybe have caught it. I should see what's going wrong. Thank you for adding the test.

@domoritz
Copy link
Member

Hmm, the coverage upload works on my branches: https://app.circleci.com/jobs/github/vega/ts-json-schema-generator/192.

@domoritz
Copy link
Member

Ahh, @codecov doesn't work well with circleci. They don't allow coverage unless you run on the same fork.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Oct 23, 2019 via email

@domoritz domoritz merged commit 2f210d6 into vega:master Oct 24, 2019
@domoritz
Copy link
Member

Thank you!

@bollwyvl
Copy link
Contributor Author

Thanks again for the great tool!

Over on The Quest for LSP, I finally made some progress, and am now generating 227 (minus 51 i made up, and one that wasn't exported) types, including voids!. This (almost) covers the Request side of the the house with just a few remaining gotchas. Hopefully I'll get to start on the Responses soon, i'm sensing some typeofs will be needed, which doesn't sound very fun...

@bollwyvl
Copy link
Contributor Author

Over on The Quest for LSP

Actually, that worked out much better than I could have hoped, and is basically done: thanks for making it possible!

https://gist.github.com/bollwyvl/7a128978b8ae89ab02bbd5b84d07a4b7

There's a nagging issue, no doubt with my kludgy generated typescript around a oneOf/anyOf I'm not asking for yet, but otherwise looking pretty good!

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.

2 participants