Skip to content

#2987 Support for defining Javadoc in the generated mapper implementation #3219

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.

Sign up for GitHub

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

Jump to bottom
Merged
filiphr merged 21 commits into from
May 1, 2023
Merged

#2987 Support for defining Javadoc in the generated mapper implementation #3219

filiphr merged 21 commits into from
May 1, 2023

Conversation

@jccampanero
Copy link
Contributor

@jccampanero jccampanero commented Mar 28, 2023 •
edited by filiphr
Loading

The proposed change provides a new annotation allowing the definition of Javadoc comments in the generated Mapper implementation.

For instance, the following definition:

@Javadoc(
    value = "This is the description",
    authors = { "author1", "author2" },
    deprecated = "Use {@link OtherMapper} instead",
    since = "0.1"
)

will generate something like:

/**
* This is the description
* 
* @author author1
* @author author2
* 
* @deprecated Use {@link OtherMapper} instead
* @since 0.1
*/

The annotation allows users to write Text blocks:

@Javadoc(
    """
    This is the description
                   
    @author author1
    @author author2
                   
    @deprecated Use {@link OtherMapper} instead
    @since 0.1
    """
)

or plain old Java Strings:

@Javadoc("This is the description\n"
                   + "\n"
                   + "@author author1\n"
                   + "@author author2\n"
                   + "\n"
                   + "@deprecated Use {@link OtherMapper} instead\n"
                   + "@since 0.1\n")

Fixes #2987.

filiphr
filiphr requested changes Apr 22, 2023
View reviewed changes
Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

This looks really good @jccampanero. Thanks a lot for your work on this and sorry that it took me so long to review it.

I've left some comments that I think we should address before we merge this to main.

In addition to the comments. It would perhaps be good to add something in the documentation for this and to also add something in the javadoc of @Mapper, perhaps a @see.

What do you think @jccampanero

@jccampanero
Copy link
Contributor Author

jccampanero commented Apr 22, 2023

This looks really good @jccampanero. Thanks a lot for your work on this and sorry that it took me so long to review it.

I've left some comments that I think we should address before we merge this to main.

In addition to the comments. It would perhaps be good to add something in the documentation for this and to also add something in the javadoc of @Mapper, perhaps a @see.

What do you think @jccampanero

Thank you very much @filiphr.

There is no need to apologize; on the contrary, I am sorry for the time in providing a pull request.

In addition, thank you very much for your time and for reviewing the code.

I have been working in the changes you proposed: only the tests are still pending, and probably improving the Javadoc comments of the Javadoc annotation.

Of course, I agree with you: I will include a reference to the new functionality in the Javadoc comments of Mapper and in the library documentation. What section of the documentation do you think would be best suited to define the new functionality?

@sjaakd
Copy link
Contributor

sjaakd commented Apr 23, 2023

Nice feature 👍

filiphr
filiphr reviewed Apr 23, 2023
View reviewed changes
Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

The comments in the Javadoc look good to me.

I've left one small comment for the printing of the error message if something is missing. You could also add an erroneous test for that as well.

For the documentation, I think that we can add an additional section to chapter-3-defining-a-mapper.asciidoc below the "Adding annotations" section, i.e. at the end.

@filiphr
Copy link
Member

filiphr commented Apr 30, 2023

Thanks for the changes @jccampanero. I've pushed one small polishing commit for the tests. Can you please add some docs and then we can merge this?

@jccampanero
Copy link
Contributor Author

jccampanero commented Apr 30, 2023

Thank you very much for the changes performed in the tests @filiphr.

Of course, I will update the documentation to include the Javadoc functionality.

Can I update JavadocTest to use fixtures as you asked me to do too?

@filiphr
Copy link
Member

filiphr commented Apr 30, 2023

Can I update JavadocTest to use fixtures as you asked me to do too?

I forgot about this. Yes please go ahead and update that test to use fixtures.
I did the polishing thinking that I'll merge it immediately, but then I saw that the documentation was not there yet.

@jccampanero
Copy link
Contributor Author

jccampanero commented Apr 30, 2023

Thank you very much @filiphr.

Yes, sorry, I have been busy the last week.

@filiphr
Copy link
Member

filiphr commented Apr 30, 2023

Yes, sorry, I have been busy the last week.

No need to apologise @jccampanero. We are all doing this in our free time, as you can see I am also not the most free person as well 😄

filiphr
filiphr reviewed Apr 30, 2023
View reviewed changes
Copy link
Member

@filiphr filiphr left a comment

Choose a reason for hiding this comment

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

Only one small typo in the docs

@jccampanero
Copy link
Contributor Author

jccampanero commented Apr 30, 2023

I implemented the required changes, the one related to the documentation and the other one related to the use of fixtures in the tests.

@filiphr, please, consider review carefully the fixtures provided: I think they are right but I was unable to run the tests in IntelliJ, for any reason the fixtures directory are not being copied to the test output folder in my local setup.

Should we create additional test cases?

@jccampanero
Copy link
Contributor Author

jccampanero commented Apr 30, 2023

Yes, sorry, I have been busy the last week.

No need to apologise @jccampanero. We are all doing this in our free time, as you can see I am also not the most free person as well 😄

Thank you very much always for your kindness @filiphr, I really appreciate it.

@jccampanero
Copy link
Contributor Author

jccampanero commented Apr 30, 2023

@filiphr I would like to perform an additional change before closing the PR if you consider it appropriate.

The generated Javadoc comments always start right straight the new line character, no matter how I configure the FreeMarker template. I mean, it generates:

/**
*

but I think this should be:

/**
 *

Note the space before the asterisk.

Please, can you advice me about how I should modify the FreeMarker template to deal with that situation? Thank you in advance.

@jccampanero
Copy link
Contributor Author

jccampanero commented Apr 30, 2023

@filiphr I would like to perform an additional change before closing the PR if you consider it appropriate.

The generated Javadoc comments always start right straight the new line character, no matter how I configure the FreeMarker template. I mean, it generates:

/**
*

but I think this should be:

/**
 *

Note the space before the asterisk.

Please, can you advice me about how I should modify the FreeMarker template to deal with that situation? Thank you in advance.

After reviewing the code I think the issue could be motivated by org.mapstruct.ap.internal.writer.IndentationCorrectingWriter and the way it seems to handle whitespace at the beginning of new lines.

If I'm right, I think it's preferable to use the current implementation of the code, as it generates correct Javadoc comments, which is ultimately the only important thing in this case, to avoid changing the current functionality implemented for the rest of the templates.

@filiphr
Copy link
Member

filiphr commented May 1, 2023

After reviewing the code I think the issue could be motivated by org.mapstruct.ap.internal.writer.IndentationCorrectingWriter and the way it seems to handle whitespace at the beginning of new lines.

You are right @jccampanero. The IndendationCorrectingWriter is the one which is removing the whitespaces. We'll need to do something special there to properly handle this. I would say that for now it is OK how it looks like. We can create a separate issue for this if you want.

@filiphr filiphr merged commit a8df94c into mapstruct:main May 1, 2023
@filiphr
Copy link
Member

filiphr commented May 1, 2023

This has been integrated. Thanks @jccampanero

@jccampanero jccampanero deleted the 2987_support_for_javadoc_in_generated_mapper branch May 2, 2023 21:37
@jccampanero
Copy link
Contributor Author

jccampanero commented May 2, 2023

You are welcome, and thank you for your support @filiphr.

Let's tackle the next issue!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@filiphr filiphr filiphr requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support for defining Javadoc in the generated mapper implementation

3 participants

@jccampanero @sjaakd @filiphr