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 conflicting in POJOPropertiesCollector when having namingStrategy #2979

Closed
wants to merge 3 commits into from

Conversation

xiaosunzhu
Copy link

swap _renameUsing and trimByVisibility in collectAll().

in _renameUsing will check conflict, but trimByVisibility will filter some conflict, so trimByVisibility before _renameUsing.

…ving namingStrategy.

swap _renameUsing and trimByVisibility. in _renameUsing will check conflict, but trimByVisibility will filter some conflict, so trimByVisibility before _renameUsing.
@cowtowncoder
Copy link
Member

@xiaosunzhu First of all, thank you for submitting this patch!

I think I would need at least one test case to show a problem that is resolved by this change (I assume all existing unit tests pass with and without change). This may make sense, but given that the logic is quite sensitive to ordering, I would want to be sure of case(s) being resolved.

@xiaosunzhu
Copy link
Author

@cowtowncoder
Ofcourse. I added 2 test cases.

This exception only appears in specific situation, and can also be avoided by modifying code, but logically, it may be better to adjust the order.
I am not sure whether it will cause other problems.

@cowtowncoder
Copy link
Member

Unfortunately it looks like you reformatted the whole test for some reason and I cannot see the diff in the test case. I would specifically want to see which 2 tests were added.

But I also think that what I would need is an example of user-level problem being solved: it is fine to also test POJOPropertiesCollector, but I would want to know at higher level kind of case where ordering of renaming, removal matter.
This because changes in this area have commonly caused problems with some edge case not yet covered by testing, so I want to make sure the problem being solved is real.

@xiaosunzhu
Copy link
Author

my fault. I revert farmat.

@cowtowncoder
Copy link
Member

Thank you for the changes, including test case!

@cowtowncoder cowtowncoder added this to the 2.12.1 milestone Dec 22, 2020
@cowtowncoder
Copy link
Member

@xiaosunzhu Thank you for the fix -- I ended up changing the test slightly (to produce problem from use case, not testing POJOPropertiesCollector), and manually merging -- but the fix is the same.

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.

2 participants