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

Feature/methods #121

Closed
wants to merge 8 commits into from
Closed

Feature/methods #121

wants to merge 8 commits into from

Conversation

heikomat
Copy link

@heikomat heikomat commented Jun 5, 2017

Please:

  • Do not include the compiled .js, .js.map, or .d.ts in your pull request as it makes it harder to merge your changes.
  • Make your pull request atomic, fixing one issue at a time unless there are many relevant issues that cannot be decoupled.
  • Provide a test case & update the documentation in the Readme.md

This implements #110.

This is probably not optimal, as i've not studied the JSON-Schema-specs in depth, but i've talked to @5ebastianMeier, and this would work for him.

If anything needs to be changed, please let me know, i'm happy to adjust it if necessary, especially because i think i haven't reused as much of the existing code as i could have.

{
"name": "d",
"type": "union",
"typeArguments": [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with this syntax. Can you send me some resources so I can evaluate this?

Copy link
Author

@heikomat heikomat Jun 5, 2017

Choose a reason for hiding this comment

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

@domoritz: TypeScript - generics

  • the string in Array<string> is a typeArgument
  • the implements in T implements Array<string> makes Array<string> a constraint (on T)
  • for union-types and intersection-types in method-definitions i used typeArguments to represent the types that are part of that union/intersection.

I unfortunately don't really have sources on how a function should be represented in a JSON-Schema, because there barely are sources. I've read somewhere that JSON-Schema isn't really meant to include functions, so i somewhat came up with my own way to represent them.

If there happens to be standards for this, that i just overlooked, i'm more than happy to follow it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know Generics in typescript but this syntax in JSON schema strikes me as an odd use case for JSON schema. A schema is usually intended for data structures and not really for functions or method signatures.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, usually you wouldn't use JSON-Schema for that, because a function has functionality. But the use case here is not to verify a whole function (including the implementation), but to verify it's declaration. The goal is to compare "functions" based on their name, parameter, parameter-order, parameter-types, return-type etc.

I think JSON-Schema can be a good tool to verify function-declarations, without comparing the function-implementation.

@domoritz
Copy link
Collaborator

domoritz commented Jun 5, 2017

I personally think this is too non-standard and I would like to keep the library lean. Unless somebody has strong opinions, I vote for not including this. Can we somehow find a way to implement a plugin interface so that you can easily extend the ts to json schema converter instead?

@heikomat
Copy link
Author

heikomat commented Jun 5, 2017

@domoritz I see your point. I'd be completely happy with a plugin-interface that would allow me to add this functionality!

@domoritz
Copy link
Collaborator

domoritz commented Jun 5, 2017

@heikomat Thanks! I understand that you put some effort into this PR.

Unfortunately, I won't have the cycles to add the plugin interface but maybe someone else can chime in. I'm closing this PR for now.

@domoritz domoritz closed this Jun 5, 2017
@heikomat
Copy link
Author

heikomat commented Jun 5, 2017

@domoritz i'm not toooo familiar with typescript-json-schema (i guess you can can see my level of knowledge about this library in this PR), but if it's ok, i could give adding a plugin interface a try in another branch

@5ebastianMeier
Copy link

@domoritz I understand your reason to not merge the PR now. The JSON schema standard doesn't specify methods or generics by default. But the standard allows (and encourages) expanding it. The basic type schema for functions and promises could therefore easily be supplied in additional JSON schemas, that we can later reference in our generated JSON schemas.

But I also agree that it shouldn't be considered the default behavior of this library as this is beyond the core JSON schema standard.

I hope that we can revisit this as a plugin as soon as that feature progresses.

@domoritz
Copy link
Collaborator

domoritz commented Jun 5, 2017

@5ebastianMeier Thanks for the understanding. I agree with the points you are making.

@bergundy
Copy link
Contributor

Hi,
Is there any progress with a plugin system?
What functionality would this system provide?
Anything I can help with?

@domoritz
Copy link
Collaborator

@bergundy Not that I know of. Thanks for offering to help! You can look into how we could create a plugin interface.

@bergundy
Copy link
Contributor

@domoritz I'll try and see if I can come up with.
I only have one use case in mind ATM so I don't know if I'd be too helpful with defining the plugin system.

Any ideas as of what we'd like to support?

@domoritz
Copy link
Collaborator

Not at the moment, no.

@5ebastianMeier
Copy link

@bergundy I haven't got the time right now, to write down the use cases we would need in the plugin API to implement this feature. But if you want to look into it, we've continued some further development on our fork after the initial PR (see https://github.com/essential-projects/typescript-json-schema).

Maybe the commits there can help you to isolate an API out of our changes.

Just out of curiosity: do you specifically want to implement the feature described in this PR or do you have other use cases or PRs in mind that I don't know of?

I'll gladly review any plugin concept and provide feedback from our point of view if you want.

Heads up: we're not planning to implement any further features except the one described in this PR

bergundy added a commit to bergundy/typescript-json-schema that referenced this pull request Mar 25, 2018
@bergundy
Copy link
Contributor

@5ebastianMeier I mainly need the feature in this PR, I'm planning on creating some sort of "reverse swagger" tool, and I'd like to avoid forking TJS.
I started creating a way to extend TJS but unfortunately my time is limited so progress has been slow.
I'll report back when I have something significant to show.
And thanks for the reference to the fork.

bergundy added a commit to bergundy/tjs-methods that referenced this pull request May 29, 2018
@bergundy bergundy mentioned this pull request May 29, 2018
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.

4 participants