rmannibucau commented on pull request #84: URL: https://github.com/apache/openjpa/pull/84#issuecomment-948866243
I have two points to check on this PR - maybe some misunderstanding from my side: 1. Guess the test must have asserts otherwise the feature can be silently broken and test does not play its role IMHO (I understand it was failing before the test but still). Probably checking the auto assignment from the trigger was done after a flush is sufficient. 2. we have 2 toggles (let say A and B), in the PR you do "if A then !B" but why not "if B then !A" so it looks a bit unfair to me. In other words, if the user explicitly set one of both, the other can be implied, if both are set then I guess both must be respected - potentially with a warning since I agree they overlap but user forced a config so we must respect it - and if none is set using an implicit fallback sounds better than current behavior. I don't recall if we can migrate from `boolean` to `Boolean` these two (with a potentially defaulting in the accessor) but it sounds like an option to solve this issue. Another one can be to use the setter maybe? Hope it makes sense. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@openjpa.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
