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

refactor: replace json_minimal with serde_json #235

Merged
merged 1 commit into from
Oct 13, 2022

Conversation

NeverRaR
Copy link
Contributor

@NeverRaR NeverRaR commented Oct 11, 2022

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

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

kclvm/runtime/src/3rdparty/json_minimal
kclvm/runtime/src/json/json.rs

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

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

test cases in kclvm/runtime/src/value/val_json.rs: test_value_from_err_json() is used to test decode invalid json

6. Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@Peefy
Copy link
Contributor

Peefy commented Oct 11, 2022

Pls add more test cases for val_json.rs including all functions and cases in #201

@Peefy Peefy added bug Something isn't working runtime Issues or PRs related to kcl runtime including value and value opertions fix labels Oct 11, 2022
@Peefy Peefy added this to the v0.4.4 Release milestone Oct 11, 2022
@NeverRaR NeverRaR force-pushed the dev/boying/kclvm_runtime/json branch 3 times, most recently from 2c648ae to 07e0888 Compare October 11, 2022 08:50
Peefy
Peefy previously approved these changes Oct 11, 2022
Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

@NeverRaR NeverRaR force-pushed the dev/boying/kclvm_runtime/json branch 3 times, most recently from b572f0a to a78f17c Compare October 12, 2022 05:37
@NeverRaR NeverRaR force-pushed the dev/boying/kclvm_runtime/json branch from a78f17c to 8e5101b Compare October 12, 2022 05:41
@coveralls
Copy link
Collaborator

coveralls commented Oct 12, 2022

Pull Request Test Coverage Report for Build 3232299325

  • 273 of 367 (74.39%) changed or added relevant lines in 5 files are covered.
  • 447 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.6%) to 60.417%

Changes Missing Coverage Covered Lines Changed/Added Lines %
kclvm/runtime/src/stdlib/builtin.rs 0 1 0.0%
kclvm/runtime/src/value/api.rs 0 1 0.0%
kclvm/runtime/src/json/json.rs 0 3 0.0%
kclvm/runtime/src/value/val_json.rs 272 361 75.35%
Files with Coverage Reduction New Missed Lines %
kclvm/runtime/src/value/val_json.rs 3 73.87%
kclvm/sema/src/resolver/node.rs 9 75.23%
kclvm/sema/src/ty/mod.rs 35 66.02%
kclvm/sema/src/resolver/config.rs 42 79.61%
kclvm/compiler/src/codegen/llvm/context.rs 358 74.15%
Totals Coverage Status
Change from base Build 3226038526: 0.6%
Covered Lines: 22030
Relevant Lines: 36463

💛 - Coveralls

Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zong-zhe zong-zhe left a comment

Choose a reason for hiding this comment

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

LGTM

@NeverRaR NeverRaR merged commit 436f8a3 into kcl-lang:main Oct 13, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working fix runtime Issues or PRs related to kcl runtime including value and value opertions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] json.decode raise an strange error when input invalid json string
4 participants