Skip to content

fix: default value handling for optional#3854

Merged
filiphr merged 12 commits intomapstruct:mainfrom
MelleD:fix-null-handling-for-optional
May 25, 2025
Merged

fix: default value handling for optional#3854
filiphr merged 12 commits intomapstruct:mainfrom
MelleD:fix-null-handling-for-optional

Conversation

@MelleD
Copy link
Contributor

@MelleD MelleD commented May 13, 2025

This pull request introduces support for handling not nullable Optional types in the MapStruct framework and includes related test cases. The most significant changes involve adding a new method to check for Optional types, modifying the handling of null values, and introducing test classes to validate the new functionality. Now the default is Optional.empty() instead of null

Fix #3852

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 solution looks OK. However, there are way too many whitespace changes in the Type.java, can you please revert all those changes and only commit changes that are related to the issue. Makes it easier to review and merge.

@MelleD
Copy link
Contributor Author

MelleD commented May 13, 2025

The solution looks OK. However, there are way too many whitespace changes in the Type.java, can you please revert all those changes and only commit changes that are related to the issue. Makes it easier to review and merge.

But looks more that the file is broken and not formatted with you own code style.

I used this one and saved the file:
https://github.com/mapstruct/mapstruct/blob/main/etc/mapstruct.xml

Thats the relevant change:
https://github.com/mapstruct/mapstruct/pull/3854/files#diff-d8e01412894861d5bcf25630d8ca245bc9503e797b85b0ca30695a5f07e3f491R371

https://github.com/mapstruct/mapstruct/pull/3854/files#diff-d8e01412894861d5bcf25630d8ca245bc9503e797b85b0ca30695a5f07e3f491R1170

But yes I could revert, but the next one get same issue with the code style, because the change with the spaces looks right, or?

MelleD added 2 commits May 13, 2025 17:05
Remove empty line
fix if condition
@filiphr
Copy link
Member

filiphr commented May 13, 2025

But looks more that the file is broken and not formatted with you own code style.

I need to update the formatter, I've noticed that there are some changes in newer IntelliJ versions, also it is good practice to only format new code, at least that's what I try to do. Thanks for removing the whitespaces

@MelleD
Copy link
Contributor Author

MelleD commented May 13, 2025

But looks more that the file is broken and not formatted with you own code style.

I need to update the formatter, I've noticed that there are some changes in newer IntelliJ versions, also it is good practice to only format new code, at least that's what I try to do. Thanks for removing the whitespaces

Mhm ok. For me it’s good practice in IntelliJ to use proper plugins like save actions and then have a clean format for the whole file and not only partly

@MelleD
Copy link
Contributor Author

MelleD commented May 14, 2025

@filiphr done

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.

thanks for the adjustments @MelleD. I've left few more comments, let me know if you disagree with some of them.

If you don't have time, I can wrap it up as well

@MelleD
Copy link
Contributor Author

MelleD commented May 19, 2025

thanks for the adjustments @MelleD. I've left few more comments, let me know if you disagree with some of them.

If you don't have time, I can wrap it up as well

No it's fine. I catched up and should fixed all (hopefully :) ). Please have a look again

@filiphr filiphr merged commit 05f27e9 into mapstruct:main May 25, 2025
6 checks passed
@filiphr
Copy link
Member

filiphr commented May 25, 2025

I polished it a bit and merged it. Thanks @MelleD

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.

Initialize Optional with Optional.empty instead of null

2 participants