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

#234 support for references for arrays #238

Merged
merged 3 commits into from
Mar 5, 2019
Merged

Conversation

243826
Copy link
Contributor

@243826 243826 commented Feb 27, 2019

Fixed Issue #234 by resolving the unresolved references.

@243826
Copy link
Contributor Author

243826 commented Feb 28, 2019

Pardon my whitespace changes. It seems that the file was badly formatted and my editor reformatted it even before i started making changes. I kept the changes since it looked like the changes were for the betterment...

but open to suggestions.

@stevehu
Copy link
Contributor

stevehu commented Feb 28, 2019

@243826 Thanks for the contribution. No worry about the whitespaces. As we are not using array references, I am wondering if you could add one or more unit tests to demo how to use it and these test can greatly help us to understand your code.

Copy link
Contributor

@ddobrin ddobrin left a comment

Choose a reason for hiding this comment

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

Hi @243826 : my current client has array references in a number of OpenAPI specs.

Could you please add a test which employs a (simple) OpenAPI spec showcasing arrays?

Thank you

Copy link
Contributor

@jiachen1120 jiachen1120 left a comment

Choose a reason for hiding this comment

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

@243826 Really an awesome solution, I learned a lot from it. Thank you! Also, there are some suggestions I mentioned in the review.

@243826
Copy link
Contributor Author

243826 commented Feb 28, 2019

@243826 Thanks for the contribution. No worry about the whitespaces. As we are not using array references, I am wondering if you could add one or more unit tests to demo how to use it and these test can greatly help us to understand your code.

yes, I think I should... I was just being lazy:

  1. Since the openapi.json already has array of NotificationPreference, it's a positive test.
  2. I did not spend enough time to understand how your tests validate that the generated code is sane. Do you have pointers to understand it quickly?

@243826
Copy link
Contributor Author

243826 commented Feb 28, 2019

Hi @243826 : my current client has array references in a number of OpenAPI specs.

Could you please add a test which employs a (simple) OpenAPI spec showcasing arrays?

Thank you

Is it possible for you to share a stripped down version of a openapi3 specification for your client's API? It will save me valuable time to think of an example which most likely will be madeup and meaningless.

@stevehu
Copy link
Contributor

stevehu commented Feb 28, 2019

@243826 We don't have end-to-end verification for the codegen at the moment in light-codegen; however, we have a lot of examples in light-example-4j and specification/config in model-config repo. Each time we do a build, we are testing some of the examples end-to-end in light-bot. We can easily expand that to generate some example applications from specifications and start the server to test against the example response. I have opened an issue in light-bot. networknt/light-bot#61

@243826
Copy link
Contributor Author

243826 commented Mar 1, 2019

I have added a unit test which validates the affected code generation. I also addressed the enum side of the story. Please review and let me know.

Also just to share with you guys - I am re-architecting a large code base and had laid out principles as to what the new architecture should support. I was mainly looking for a fast microservices framework when I came across light-4j and was delighted to see that many of my principles match with how you guys laid out on your site. Keep up the good work. Let me know if you would like me to review new ideas. Since I am trying to use your framework, I am sure to come by to review the progress of this product.

@stevehu
Copy link
Contributor

stevehu commented Mar 2, 2019

@243826 Welcome to the team. I have sent you an invite to add to the contributor list. Please feel free to join any discussions and review any issues. Once the team members know which repo or area you are interested in, you will be added to the reviewer list by them :)

@243826
Copy link
Contributor Author

243826 commented Mar 4, 2019

@ddobrin I have addressed your change request as well. Could you validate it and/or approve the pull request so that it can be merged? Thanks.

@243826
Copy link
Contributor Author

243826 commented Mar 5, 2019

@stevehu Could you please look into merging this ASAP?

@stevehu stevehu changed the base branch from master to develop March 5, 2019 20:20
@stevehu stevehu merged commit 9fe618f into networknt:develop Mar 5, 2019
@243826
Copy link
Contributor Author

243826 commented Mar 5, 2019

what's the significance of develop branch vs master branch?

@stevehu
Copy link
Contributor

stevehu commented Mar 5, 2019

We release from the develop branch and then merge the develop to master. So that master branch is in sync with our latest release.

@243826
Copy link
Contributor Author

243826 commented Mar 5, 2019 via email

@stevehu
Copy link
Contributor

stevehu commented Mar 5, 2019

Yes. All developers should work against the develop branch. Once the PR is submitted, we need to have two reviewers to approve the PR before merging into the develop branch for the next release.

This is the contribution document. https://doc.networknt.com/contribute/development/

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