vy commented on pull request #444: URL: https://github.com/apache/logging-log4j2/pull/444#issuecomment-735686828
First and foremost, thanks so much @carterkozak for reviewing the changes and thinking along for improvements. I have really appreciated them. `ParameterizedMessage.deepToString()` changes seem legitimate to me. That said, I would like to share a few opinions of mine triggered by this change set: 1. Even though I was the one who started using `ParameterizedMessage.deepToString()`, thinking about it again, I am not happy with the end result. This breaks the self-containment contract of the class. I would/might rather copy your (or all?) optimizations from `ParameterizedMessage.deepToString()` to somewhere in `JsonWriter#writeString()`. 2. Indeed it smells awkward to have JSON quoting routines in multiple locations within the code base. I had shared my concerns about it in [a mailing list thread](https://lists.apache.org/thread.html/r2c20464cf815ff16e6efbf26829db3b3116da16f914e7aa9d6afacc8%40%3Cdev.logging.apache.org%3E). But in a nutshell, why do we even have JSON quoting routines anywhere in the code base except `JsonWriter`? `JsonLayout` already uses Jackson, hence it is covered. But for the rest... I think we should just keep them for backward compatibility and remove them in `master`. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
