kbendick edited a comment on pull request #3543: URL: https://github.com/apache/iceberg/pull/3543#issuecomment-991980796
> Overall I like the approach of using interval to decide if cache is enabled or not, I have 2 major discussion points around the implementation. > > Another thing is about the style. I feel there are too many javadocs and line comments, usually more javadoc is recommended to improve the experience of other developers, but I feel a bit lost with that many explanations, I don't know if it's just me or if others have similar feelings. I think the code you wrote is already pretty self-explanatory (which is great), and reading the code is faster for me to understand comparing to reading the comments. I would suggest: > > 1. make comments in tests just messages in assertions if you would like people to know what you are testing > 2. if possible, make the code itself more readable, and only add comments when additional explanation is needed I agree about the comments. Especially in the tests. A lot of the test comments came from a request to use Assert4J (which definitely made sense), which I'm not quite familiar working with. Some of the tests comments are simply not needed and will be removed. Others I'll either leave in or switch to something that allows for an assertion message. Thanks for your feedback. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
