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 JsonPropertyOrder with child annotations #423

Conversation

magicDGS
Copy link
Contributor

I have added a test to demonstrate the issue that was failing before the actual change. The result that was given before the fix was:

{
  "$schema" : "https://json-schema.org/draft/2019-09/schema",
  "type" : "object",
  "properties" : {
    "3_third" : {
      "type" : "integer",
      "description" : "My integer description"
    },
    "1_first" : {
      "type" : "string",
      "description" : "My string description"
    },
    "2_second" : {
      "type" : "string",
      "description" : "My second string description"
    }
  }
}

I tried to have a workaround by extending the JsonPropertySorter and override the getPropertyIndex, but I couldn't access the propertyOrderPerDeclaringType on the custom implementation. After checking the rest of the test, I guess that it should not cause any regression.

Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

Code Health Quality Gates: OK

  • Declining Code Health: 0 findings(s) 🚩
  • Improving Code Health: 0 findings(s) ✅
  • Affected Hotspots: 0 files(s) 🔥

Recommended Review Level: Detailed -- Increased risk for defects: The risk is higher as much of the code in this repo (99% of all commits) is written by other authors.
View detailed results in CodeScene

Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

Code Health Quality Gates: OK

  • Declining Code Health: 0 findings(s) 🚩
  • Improving Code Health: 0 findings(s) ✅
  • Affected Hotspots: 0 files(s) 🔥

Recommended Review Level: Detailed -- Increased risk for defects: The risk is higher as much of the code in this repo (99% of all commits) is written by other authors.
View detailed results in CodeScene

@magicDGS
Copy link
Contributor Author

@CarstenWickner - could it be possible in case that this is approved to cut a new version of the library. I need it to proceed proceed with some implementation on the project where this was found, so I would really appreciate if it is possible. Thanks a lot!

@CarstenWickner CarstenWickner changed the base branch from main to jsonpropertysorter-fix December 18, 2023 19:48
Copy link
Member

@CarstenWickner CarstenWickner left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution.
I'll merge this to a feature branch first, to make it a bit more consistent with the existing code (mostly formatting).
I aim to create a new release soon.

@CarstenWickner CarstenWickner merged commit 179a9d5 into victools:jsonpropertysorter-fix Dec 18, 2023
11 checks passed
CarstenWickner added a commit that referenced this pull request Dec 18, 2023
* fix: JsonPropertyOrder with child annotations (#423)

* chore: clean up test

* chore(docs): update CHANGELOG

* chore(docs): update contributors list

* chore: harmonise indentations

---------

Co-authored-by: Daniel Gómez-Sánchez <7352559+magicDGS@users.noreply.github.com>
@CarstenWickner
Copy link
Member

Hi @magicDGS,

I've just released this change as v4.33.1.
It may take a couple of hours to fully propagate everywhere.

@magicDGS
Copy link
Contributor Author

Thank you very much, @CarstenWickner - both the software and you as maintainer are great for the java ecosystem ☺️

@magicDGS magicDGS deleted the magicDGS/fixJsonPropertyOrderWithChildAnnotations branch December 19, 2023 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants