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

support nested collections (issue #5) #8

Closed
wants to merge 3 commits into from
Closed

support nested collections (issue #5) #8

wants to merge 3 commits into from

Conversation

sojournerc
Copy link

This was a quick and dirty implementation, and could probably use a little refactor for readability.

Took the recursive approach, just had to break apart the methods to be able to recurse the right step in the process.

@sojournerc
Copy link
Author

resolves #5

@Ajaxy
Copy link
Owner

Ajaxy commented Nov 4, 2018

@sojournerc Thanks for the PR! Looks good to me. However, wouldn't be easier just to count []-s at the end and return some kind of collectionDimension from the parseType function? Then we can do just:

  if (isCollection) {
    for (let i = 0; i < collectionDimension; i++) {
      def = {
        type: 'array',
        items: def,
      };
    }
  }

@sojournerc
Copy link
Author

@Ajaxy thanks for the feedback.

I've been playing with that approach and it has some limitations:

  • The nested ref doesn't get validated (b/c the nested property doesn't get passed through parseSingleProp as it does with the recursive approach.
  • For the same reason the nested ref also doesn't get processed correctly into the swagger format

I could do some surgery on parseSingleProp to make those possible for every dimension, but I'm not sure it'd be any clearer/better than the current approach.

Thoughts?

@Ajaxy
Copy link
Owner

Ajaxy commented Nov 5, 2018

@sojournerc Not sure what you mean by "doesn't get validated". Here is a draft of how I see it may be done, your tests pass: https://github.com/Ajaxy/tinyspec/pull/10/files

@sojournerc
Copy link
Author

sojournerc commented Nov 6, 2018

Ha, that is much easier and cleaner. I got stuck in the idea of recursion, and didn't think to pull all the brackets off the type def.

Anyway, thanks again. I'll close this PR since yours is better and ready.

@sojournerc sojournerc closed this Nov 6, 2018
@Ajaxy
Copy link
Owner

Ajaxy commented Nov 8, 2018

Merged that one. Thank you too!

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