Skip to content

Adding json format for import #766

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

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

mathispernias
Copy link
Contributor

  • Tests pass
  • ruff format
  • README.md updated (if relevant)
  • CHANGELOG.md entry added

@mathispernias mathispernias changed the title Json import Adding json format for import May 20, 2025
CHANGELOG.md Outdated
@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [0.10.25] - 2025-05-07

### Added
- `datacontract import --format json`: Import from JSON files
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this to

Unreleased

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

mermaid2.html Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not belong here, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my mistake i removed this file

query Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it!

@@ -0,0 +1,216 @@
dataContractSpecification: 1.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this datacontract.yaml used?
Why has it such as generic name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I forgot to remove it, it's done!

@@ -0,0 +1,5002 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

please move these files to a subdirectory fixtures/import/json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay it's done

@@ -0,0 +1,2301 @@
dataContractSpecification: 1.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

please rename to product_detail.datacontract.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it

@@ -0,0 +1,317 @@
dataContractSpecification: 1.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

please rename to productsimple.datacontract.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it

@jochenchrist
Copy link
Contributor

This implementation looks like it supports JSON files with single structures.
In practive, we often see JSON files with new line delimited entries that contain 1000 messages.

Example: https://github.com/datacontract/datacontract-cli/blob/main/tests/fixtures/s3-json/data/inventory/year%3D2022/month%3D04/day%3D20/hour%3D00/inventory%2B0%2B0001327496.json

Can you please make sure that the JSON importer also supports JSON documents with multiple entries?

@mathispernias
Copy link
Contributor Author

The JSON importer can now also takes NDJSON (Newline-delimited JSON).

@simonharrer
Copy link
Contributor

I like it. Can you make the test jsons a little smaller? And make sure the test jsons and their imported contracts have the same file prefix.

@mathispernias
Copy link
Contributor Author

I updated the JSON tests to include three cases—simple JSON, complex JSON, and NDJSON— I also changed the file prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty file?

Copy link
Contributor

@jochenchrist jochenchrist left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution

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