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

Move json.hpp to a peer folder of other third-party dependencies - Stage 1 #1325

Merged
merged 8 commits into from
Feb 6, 2025

Conversation

mkoscumb
Copy link
Contributor

@mkoscumb mkoscumb commented Feb 3, 2025

Beyond being good dependency hygiene, this will also allow adopters which have their own copy of json.hpp to more easily use that when integrating the 1DS C++ SDK (via include path modification)

This work will be broken down into three PRs.

  1. #This PR# Copy json.hpp from lib\include\mat => nlohmann\ update include paths accordingly.
  2. Update include paths in the private modules repo.
  3. Remove lib\include\mat\json.hpp and update the private modules repo commit to the result of Stage 2.

@mkoscumb mkoscumb marked this pull request as ready for review February 4, 2025 18:50
@mkoscumb mkoscumb requested a review from a team as a code owner February 4, 2025 18:50
CMakeLists.txt Outdated Show resolved Hide resolved
@ThomsonTan
Copy link
Contributor

The new json.hpp file seems cannot be loaded into github for review because it has too much lines.

Can we download this json.hpp file from github during build instead of copying it?

@lalitb
Copy link
Contributor

lalitb commented Feb 4, 2025

Unfortunately, macOS build is still not working, it's on me. Hopefully, this doesn't break it. We should be able to force-merge it for now.

@lalitb
Copy link
Contributor

lalitb commented Feb 4, 2025

The new json.hpp file seems cannot be loaded into github for review because it has too much lines.
Can we download this json.hpp file from github during build instead of copying it?

@ThomsonTan - It's actually copied from lib/mat directory to new location, so content needn't be reviewed. Because of 3 step process, it can't be moved from lib/mat to root directory. I think downloading from Github can be tracked separately.

@ThomsonTan
Copy link
Contributor

The new json.hpp file seems cannot be loaded into github for review because it has too much lines.
Can we download this json.hpp file from github during build instead of copying it?

@ThomsonTan - It's actually copied from lib/mat directory to new location, so content needn't be reviewed. Because of 3 step process, it can't be moved from lib/mat to root directory. I think downloading from Github can be tracked separately.

Ok, comparing large files is not very intuitive in the revie UI, but I've verified the file move.

Copy link
Contributor

@ThomsonTan ThomsonTan left a comment

Choose a reason for hiding this comment

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

:shipit:

@ThomsonTan ThomsonTan merged commit cf69613 into main Feb 6, 2025
20 of 24 checks passed
@ThomsonTan ThomsonTan deleted the mkoscumb/MoveJsonHppStage1 branch February 6, 2025 22:07
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.

3 participants