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

fix(JSON-RPC): make the json schema validation fail when result is null #958

Merged

Conversation

nagmo-starkware
Copy link
Contributor

@nagmo-starkware nagmo-starkware commented Aug 1, 2023

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information


This change is Reviewable

@nagmo-starkware
Copy link
Contributor Author

nagmo-starkware commented Aug 1, 2023

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #958 (92ce648) into connect_v0.4.0_to_api_endpoint (fbf0184) will not change coverage.
The diff coverage is n/a.

❗ Current head 92ce648 differs from pull request most recent head 5d65d31. Consider uploading reports for the commit 5d65d31 to get more accurate results

@@                       Coverage Diff                       @@
##           connect_v0.4.0_to_api_endpoint     #958   +/-   ##
===============================================================
  Coverage                           74.26%   74.26%           
===============================================================
  Files                                  63       63           
  Lines                                5157     5157           
  Branches                             5157     5157           
===============================================================
  Hits                                 3830     3830           
  Misses                                604      604           
  Partials                              723      723           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nagmo-starkware)


crates/papyrus_gateway/src/test_utils.rs line 73 at r1 (raw file):

}

// todo: nevo - schmea validates null as valid for an unknown reason should investigate in the

Split into two sentences:
Schema validates null as valid for an unknown reason. Should investigate in the future ...


crates/papyrus_gateway/src/test_utils.rs line 73 at r1 (raw file):

}

// todo: nevo - schmea validates null as valid for an unknown reason should investigate in the

Use correct TODO format and capitalization

// TODO(nevo): Schema

crates/papyrus_gateway/src/test_utils.rs line 73 at r1 (raw file):

}

// todo: nevo - schmea validates null as valid for an unknown reason should investigate in the

schmea -> Schema


crates/papyrus_gateway/src/test_utils.rs line 74 at r1 (raw file):

// todo: nevo - schmea validates null as valid for an unknown reason should investigate in the
// future and remove this function (use is_valid directly)

Instead of using should, use declarative dialect: "Investigate this and remove this function (use is_valid directly)

Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nagmo-starkware)

Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nagmo-starkware)

* feat(JSON-RPC): add support for v0.4.0-rc3

* fix(JSON-RPC): spec bugfixes that are in review on the main repo and not yet merged and tagged

* fix(JSON-RPC): more changes from product
@nagmo-starkware nagmo-starkware merged commit ad36948 into connect_v0.4.0_to_api_endpoint Aug 2, 2023
15 checks passed
@nagmo-starkware nagmo-starkware deleted the fix-json-vlidate-null-issue branch August 2, 2023 07:50
github-merge-queue bot pushed a commit that referenced this pull request Aug 2, 2023
* feat(JSON-RPC): connect v0.4.0 to api endpoint

* fix(JSON-RPC): make the json schema validation fail when result is null (#958)

* fix(JSON-RPC): make the json schema validation fail when result is null

[optional body]

[optional footer(s)]

* fix(JSON-RPC): merge fix

* feat(JSON-RPC): add support for v0.4.0-rc3 (#959)

* feat(JSON-RPC): add support for v0.4.0-rc3

* fix(JSON-RPC): spec bugfixes that are in review on the main repo and not yet merged and tagged

* fix(JSON-RPC): more changes from product
@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants