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: build new JsonLDObject of JsonLDObject with non-empty DEFAULT_JS… #11

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

Conversation

rkxx
Copy link

@rkxx rkxx commented Feb 24, 2023

The changes in the pom file are required to run the unit tests (kotlin). Feel free to only merge the source changes and keep the project pure java.

Without the correction the test createDerivedJsonLDObjectWithDefaultContexts fails, because the property contexts of the resulting jsonLDObject contains a listOf(URI, listOf(URI)) instead of a flat listOf(URI, URI)

Copy link
Member

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

Hello thanks for the PR! Two comments:

  1. I would indeed prefer to keep this pure Java rather than introducing Kotlin (especially if it's just for unit tests).
  2. In JsonLDObject this PR changes a lot of the whitespace, newlines, etc. As a consequence, it's really hard to see what "actual" change this PR is making.

Could you update the PR to leave all the whitespace and formatting as-is, and only apply the actual change you're proposing?

@rkxx
Copy link
Author

rkxx commented Feb 24, 2023

Sorry, my fault. Intellij automatically ignored all formating changes so I wasn't aware of the mess. I have removed all formatting changes and unit tests. Is there any reason that you don't accept kotlin. It's really hard for me to fall back to plain Java. I did it for the existing sources, but would prefer to add new separated code using kotlin. Nevertheless, for the time being I removed the unit tests.

@peacekeeper
Copy link
Member

Thanks, please check if you are a member of DIF and the Identifiers&Discovery Working Group, so we can accept the contribution: https://identity.foundation/join/

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.

2 participants