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 Javadoc to constructors parameters for properties that have title/description/$comment #1481

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

unkish
Copy link
Collaborator

@unkish unkish commented Feb 10, 2023

Add Javadoc to constructors parameters which have corresponding documented properties.
Properties that are not documented (do not have title or description) won't be added to constructors Javadoc.

Following format will be used:

/**
 * {@param {title. }?{description.}?}?
 */

Closes #1463

@joelittlejohn
Copy link
Owner

I know I recommended this, but I think with this change we will see a new javadoc warning:

warning: no @param for somearg

For any argument that doesn't have @param.

So we get a warning for no param, and we get a warning for param without description. So we're a bit stuck if we don't have a description 🤔 Making up some junk (like "The somearg value") seems pointless too.

@unkish
Copy link
Collaborator Author

unkish commented Feb 15, 2023

I've been thinking about it and couldn't come up with any "ideal" solutions, so far following "ideas" where on the table:

  • accept the fact that we can't do much and add junk description eg. @param somearg somearg
    • Drawback: junk in Javadoc comments
  • having more complicated wording there "The somearg value"
    • Drawback: wouldn't improve much over previous solution whilst also might be not the best solution for projects that might be using non-english descriptions ?
  • have a configuration property for missing property descriptions
    • Drawback: one more config property to support that doesn't add much value, but would "mitigate" issue of approach above - adding static complicated description text
  • have a configuration property to disable adding of Javadoc params
    • Drawback: one more config property to support
  • try to complicate logic a bit - apply "all or nothing" strategy: eg. check if number of Javadoc descriptions for constructor matches the number of arguments - add Javadoc, else skip it entirely
    • Drawback: might not be so obvious what is going on for users, why in certain cases Javadoc is generated and in the other cases is not
  • some sort of combination of above

@joelittlejohn
Copy link
Owner

joelittlejohn commented Feb 15, 2023

On reflection I think the way you have implemented this is correct. We should omit any param that has no description.

If you see a warning telling you that you have not documented some parameters, then this warning is accurate, and you can fix it by documenting the parameter (add title and/or description).

@unkish
Copy link
Collaborator Author

unkish commented Feb 15, 2023

For a moment I've thought that people might start complaining about schema being an external to their project and them being unable/not allowed to modify it.
But change wouldn't make it worse (granted that warning and not error will be emitted) as currently we produce Javadoc without @param description that also generates warning.

@joelittlejohn joelittlejohn added this to the 1.2.0 milestone Feb 15, 2023
@joelittlejohn joelittlejohn changed the title Add Javadoc to constructors parameters for documented properties Add Javadoc to constructors parameters for properties that have title/description/$comment Feb 15, 2023
@joelittlejohn
Copy link
Owner

@unkish This is the last one for 1.2.0 I think. Could you rebase and I will merge?

@unkish
Copy link
Collaborator Author

unkish commented Feb 16, 2023

An observation not related to given task.
There has been a change


However configuration is defined as:

and setTargetVersion accepts JsonSchemaExtension (which is a implementation of GenerationConfig)

Is gradle doing some trickery that this won't be failing in runtime ?

UPD: Probably is fine as configuration = project.jsonSchema2Pojo

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.

Javadoc for constructors missing "description" on parameters?
2 participants