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

feat: generate kcl schema from json schema #127

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

jakezhu9
Copy link
Contributor

@jakezhu9 jakezhu9 commented Aug 1, 2023

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

part of kcl-lang/kcl#526

2. What is the scope of this PR (e.g. component or file name):

pkg/tools/gen

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

In this pull request, I add the capability to convert from JSON schema to KCL schema. Here are the details:

  • Expanded the GenKcl function to support various file types and automatic detection of file types.

  • Integrated the jsonschema package for parsing JSON schemas. I made some adjustments to meet our needs, like making some fields public and using ordered maps in the "properties" keyword to preserve the order.

  • Added new types, such as Schema, Property, along with the necessary conversion code. I initially considered using the existing pb.KclType, but it lacked some fields like validation. So, I opted to create a new type.

  • Added template-related functions to generate KCL schema code. I have also added the test code, you can view the results from it.

Please note that some JSON schema keywords are not currently supported, but I plan to add them in the near future. Additionally, I'll be working on documentation and validation sections.

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@Peefy Peefy added the tool label Aug 1, 2023
pkg/tools/gen/types.go Outdated Show resolved Hide resolved
pkg/tools/gen/types.go Outdated Show resolved Hide resolved
@Peefy
Copy link
Contributor

Peefy commented Aug 1, 2023

Good Job! I will carefully review it later, and before that, there are some reminders:

  • Would you pls add more comments and tests for exposed structures and functions?
  • It would be even better if the PR could be split smaller PRs?
  • We recommend U can write your contribution process and ideas as an blog and sharing your labor achievements with the community and users. ❤️

pkg/tools/gen/jsonschema/README.md Outdated Show resolved Hide resolved
pkg/tools/gen/genkcl_jsonschema.go Show resolved Hide resolved
@jakezhu9
Copy link
Contributor Author

jakezhu9 commented Aug 2, 2023

Thanks for your careful review! Your feedback really helped improve my code.

Would you pls add more comments and tests for exposed structures and functions?

Certainly! It also remind me that as a package we should not make our internal type and functions public. I think we should only expose the structures and functions related to generating code, making it easier to use the package.

It would be even better if the PR could be split smaller PRs?

You are right! Small PRs are easier to understand and review. However, in this PR, most of the changes are in the jsonschema package, and it only implements basic conversion. So, I don't think it needs to be split further. But, I'll definitely consider making smaller PRs in the future.

We recommend U can write your contribution process and ideas as an blog and sharing your labor achievements with the community and users. ❤️

Good idea! I will try to record this process and share it later

For the kcl type in my code, I designed a typeInterface to support different types, reducing unnecessary fields in each type and making it simpler and clearer. I'm also working on making it more generic so that it can support more features in the future, even though it might seem a bit duplicative with the existing "pb.KclType."

As for the CI error, it's caused by a lower version of Go. If you run it locally with a version above 1.18, it should pass.

Your suggestions have been really inspiring, and if you have any more ideas, please feel free to share them with me!

@Peefy
Copy link
Contributor

Peefy commented Aug 2, 2023

Good Job! U can rebase the main branch to upgrade the KCL type API and go 1.19+ in CI.

@coveralls
Copy link

coveralls commented Aug 2, 2023

Pull Request Test Coverage Report for Build 5736081236

  • 167 of 219 (76.26%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.4%) to 59.768%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/tools/gen/types.go 18 21 85.71%
pkg/tools/gen/template.go 25 30 83.33%
pkg/tools/gen/genkcl.go 12 27 44.44%
pkg/tools/gen/genkcl_jsonschema.go 112 141 79.43%
Totals Coverage Status
Change from base Build 5735916630: 1.4%
Covered Lines: 1906
Relevant Lines: 3189

💛 - Coveralls

@Peefy Peefy merged commit 88ec03a into kcl-lang:main Aug 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support Go API for GetSchemaTypeMapping
3 participants