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.

Sign up for GitHub

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

Jump to bottom

Add a new jsonbridge which uses the new serializers #862

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Add a new jsonbridge which uses the new serializers #862

merged 6 commits into from
Jan 31, 2024

Conversation

Dacops
Copy link
Contributor

@Dacops Dacops commented Dec 2, 2023

No description provided.

@Dacops Dacops linked an issue Dec 2, 2023 that may be closed by this pull request
Add a new JSONBridge which uses the new serializers #856
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 2, 2023 •
edited
Loading

PR Preview Action v1.4.6
Preview removed because the pull request was closed.
2024-01-31 16:41 UTC

@Dacops Dacops changed the title feat(engine): move old json bridge to /old folder Add a new jsonbridge which uses the new serializers Dec 2, 2023
github-actions[bot]
github-actions bot previously requested changes Dec 2, 2023
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

engine/include/cubos/engine/assets/bridges/json.hpp Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Dec 2, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6ddd074) 48.91% compared to head (2df20e0) 48.91%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #862   +/-   ##
=======================================
  Coverage   48.91%   48.91%           
=======================================
  Files         134      134           
  Lines        7999     7999           
=======================================
  Hits         3913     3913           
  Misses       4086     4086

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot dismissed their stale review December 17, 2023 17:30

No Clang-Tidy warnings found so I assume my comments were addressed

@Dacops Dacops force-pushed the 856-add-a-new-jsonbridge-which-uses-the-new-serializers branch from 5e0473a to 3070643 Compare December 17, 2023 17:41
@Dacops Dacops marked this pull request as ready for review December 17, 2023 17:46
RiscadoA
RiscadoA requested changes Dec 23, 2023
View reviewed changes
Copy link
Member

@RiscadoA RiscadoA left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! :) Overall LGTM, just needs some error handling. Merry christmas! 🎅

engine/include/cubos/engine/assets/bridges/json.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridges/json.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridges/json.hpp Outdated Show resolved Hide resolved
engine/include/cubos/engine/assets/bridges/json.hpp Outdated Show resolved Hide resolved
@Dacops Dacops force-pushed the 856-add-a-new-jsonbridge-which-uses-the-new-serializers branch from 8bfd5ec to eabe308 Compare January 30, 2024 15:30
@Dacops Dacops requested a review from RiscadoA January 30, 2024 23:05
RiscadoA
RiscadoA reviewed Jan 31, 2024
View reviewed changes
core/lib/glad Outdated Show resolved Hide resolved
core/lib/json Outdated Show resolved Hide resolved
@RiscadoA RiscadoA mentioned this pull request Jan 31, 2024
Merged
4 tasks
@Dacops Dacops force-pushed the 856-add-a-new-jsonbridge-which-uses-the-new-serializers branch from d9bd715 to 2df20e0 Compare January 31, 2024 15:03
@Dacops Dacops merged commit fa22bcd into main Jan 31, 2024
10 checks passed
@Dacops Dacops deleted the 856-add-a-new-jsonbridge-which-uses-the-new-serializers branch January 31, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RiscadoA RiscadoA RiscadoA approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees
No one assigned
Labels
A-Engine B-Assets B-Serialization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a new JSONBridge which uses the new serializers
2 participants
@Dacops @RiscadoA