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

Add support for "contextual class names and sub-packages" #622

Closed

Conversation

q-src
Copy link

@q-src q-src commented Sep 16, 2016

We have a lot of complex schemas consisting of many objects and sub-objects. There are many sub-objects which are very similar and therefore have properties which are named the same.

See also the following example (dummy schema):

{
  "type": "object",
  "properties": {
    "someProperty": {
      "type": "object",
      "properties": {
        "someSubProperty": {
          "type": "string"
        }
      },
      "format": "translatable"
    },
    "anotherProperty": {
      "type": "object",
      "properties": {
        "someProperty": {
          "type": "object",
          "properties": {
            "anotherSubProperty": {
              "type": "string"
            }
          }
        }
      }
    }
  }
}

At the moment, jsonschema2pojo will generate the classes com.example.TheClass (root object), com.example.SomeProperty, com.example.SomeProperty_ and com.example.AnotherProperty.

With this PR the ugly _ can be avoided as follows:

  • By setting the configuration option useContextualClassNames to true the classes com.example.TheClassSomeProperty and com.example.TheClassAnotherPropertySomeProperty will be generated.
  • By setting the configuration option useContextualSubPackages to true the classes com.example.theclass.SomePropertyand com.example.theclass.anotherproperty.SomePropertywill be generated.
  • Of course, both configuration options can be set to true. However, this will lead to redundant information in class name and package name.

@q-src q-src changed the title Add support for "contextual class names and sup-packages" Add support for "contextual class names and sub-packages" Sep 16, 2016
@q-src q-src force-pushed the feature/keep-field-context branch 2 times, most recently from a7a03da to 60cd6bd Compare September 30, 2016 10:23
@q-src
Copy link
Author

q-src commented Sep 30, 2016

This PR is now ready for review / merge @joelittlejohn.

@q-src q-src force-pushed the feature/keep-field-context branch from 60cd6bd to cef14a7 Compare December 7, 2016 12:28
@q-src
Copy link
Author

q-src commented Dec 7, 2016

@joelittlejohn I've just rebased against master. Is anything else needed for merging?

@joelittlejohn
Copy link
Owner

@q-src I'll be honest, I'm not really that keen to merge this change. It introduces a handful of additional complexities into the class naming, and we already have a lot of complexity there. It makes any change or improvement to class naming even harder to action because of the ongoing requirement to support this feature.

This plugin can't support all possible variants of generating Java output. At some point, you have to accept that when you use this plugin you get output of a certain kind - we can't maintain a config option for every possible variant in naming that a user thinks up.

I won't say this change is not useful - but I'm not yet ready to take on this complexity and be responsible for maintaining it forever more. You've added some good integration test, but I can guarantee that there are already edge-cases in the interaction between this and other config options that will not have been considered and will be opened as bugs.

I'd like to give the class naming strategy used by this project an overhaul; something akin to the deterministic scheme proposed by @samskiter in #554. The change proposed here makes improvements like that a lot harder.

@q-src
Copy link
Author

q-src commented Jan 26, 2017

@joelittlejohn Thanks for your statement. I understand if you're not going to merge this PR and of course will accept this.

@samskiter
Copy link
Collaborator

samskiter commented Jan 29, 2017

Hey again @joelittlejohn , it's been a while since I contributed to this project, but nice to think about it again...

I seem to remember that #561 was (IMO) an important internal change that might help toward #554. I think it was being more representative of the JSON structure, but honestly struggling to remember a lot of what it did now! If you'd like to pick back over it with me, I'd be happy to look again :)

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