garydgregory edited a comment on issue #189: Add and use TimeValue await(), awaitTermination() and get() methods. URL: https://github.com/apache/httpcomponents-core/pull/189#issuecomment-575309825 @ok2c & @michael-o I will see your separation of concerns and raise you breaking encapsulation ;-) Let's step back, look at this from the 10,000 ft level and dive back in. Why do we even have a `TimeValue` class? Because for some completely absurd reason we are planning to release a MAJOR NEW version of a library, in 2020, based on Java 7. This means we cannot use Java 8's `java.util.Duration` class. We are actually telling people that Java 7 is so important that it is OK to create new projects that depend on Java 7 while even Java 8 is EOL. FWIW, a lof of the customers I see can't get to Java 11 fast enough. To make matters worse, we are talking of requiring Java 8 for 5.1. So as soon as that happens people are naturally going to see TimeValue instead of Duration all over our API and wonder what kind of silliness that is. Once 5.0 is out, the API is set in stone, so for playing nice with Java 8, we'd have to add Duration APIs where we have TimeValue. Not a pretty sight. So, now, back down to the bits. We've had to open up the guts of TimeValue with accessors like getDuration() and getTimeUnit() in order to play nice with API that take a long and a TimeUnit. That's not pretty and I claim this kind of pattern effectively breaks (in call sites) the encapsulation offered by the TimeValue abstraction. So instead of all call sites cracking a TimeValue egg open each time it needs to call some JRE concurrent APIs, I though it cleaner to flip the call around as this PR provides. You can see, especially in the tests, how much the call sites are cleaned up by this, that's IMO YMMV of course. If it were up to me, I'd make 5.0 depend on Java 8 and use Duration instead of TimeValue. Bam! Simple, expected, done. The comment "Please move those methods into a separate class similar to Operations in client." makes no sense to me; the whole point is to avoid cracking open a TimeValue in the first place. Thoughts?
---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For additional commands, e-mail: dev-h...@hc.apache.org
