On Thu, Aug 9, 2018 at 8:47 AM Gary Gregory <garydgreg...@gmail.com> wrote:
> > > On Thu, Aug 9, 2018 at 7:25 AM Rob Tompkins <chtom...@gmail.com> wrote: > >> >> >> > On Aug 9, 2018, at 9:17 AM, Gilles <gil...@harfang.homelinux.org> >> wrote: >> > >> > On Thu, 9 Aug 2018 08:43:34 -0400, Rob Tompkins wrote: >> >>> On Aug 9, 2018, at 8:38 AM, Gilles <gil...@harfang.homelinux.org> >> wrote: >> >>> >> >>> On Thu, 9 Aug 2018 08:22:32 -0400, Rob Tompkins wrote: >> >>>> Are we instead trying to remove the “extends” from all of those >> classes? >> >>> >> >>> I'm not sure what you mean, sorry. >> >>> >> >>> "SamplerBase" was meant has a utility/shortcut/boiler-plate in order >> >>> to copy the "rng" argument in one place, instead of having a field >> >>> in each sampler class. It is purely internal to this library (in C++, >> >>> inheritance would have been "private”). >> >> >> >> Are we trying to remove SamplerBase all together? >> > >> > That would break BC the same way. >> > I wouldn't mind since the shortcut is not really significant. >> > >> > But my reasoning would be the same: correct usage does not refer >> > to these methods so the fix could be done now, with minimal risk >> > (due to low usage of a new component). >> > >> >>>> My thought was to override the method and javadoc locally (in the >> >>>> actual subclass) so that we can give reasonable deprecation messages, >> >>>> but that’s only a thought. I’m ok with whatever. >> > >> > It's overkill in this context. >> > We shouldn't be rigid about the "no BC in minor release" rule; >> >> I’m a tad confused here. I thought that the whole point of everything >> that we do in terms of the way we version and name packages >> was specifically driven at the fact that we’re completely rigid about “no >> BC breakage in minor releases.” >> >> I’ve not been around the project long enough to know this with certainty, >> but that definitely is the vibe that I get. I would think that such RCs >> would generally get -1’s from people. >> >> Generally I’m open to whatever. I’m just operating on what I perceive to >> be precedent. >> >> @Gary - what’s the call here on the flexibility of BC incompatible >> changes in a minor release? >> > > BC is binary and we should not break stacks. You really don't want to > create jar hell for our users. Think about deep transitive dependencies. An > end use or developer is not going to care 2 cents about "correct vs > incorrect API usage" when a component depends on this depends on that and > so on, ends up breaking his or her whole build because a low level > component has decided that breaking BC was OK. > I'll also note that I am not close enough to this component to opine on the quality of the API or the workarounds discussed. My point is only "Don't create jar hell; don't break BC." Gary > > Gary > > >> > enforcing it is worse for everybody: >> > * bona fide users will be forced to modify their source and >> > recompile in order to benefit from the performance boost >> > introduced in version 1.1, and >> > * bad usage (if it exists at all) could continue unsuspected. >> > >> >>> The message would aim at the wrong target: for developers of this >> >>> library, there is no deprecation (the boiler-plate code is there >> >>> to be used); for application developers, the class should not be >> >>> there to be used (hence: package private). >> >> >> >> Then, I guess, we should simply state that we want to eventually >> >> delete these methods and they should not be consumed. Yeah? >> > >> > No (cf. above); I propose to delete them *now*. >> > >> >> Like I >> >> said…I’m up for whatever. I’m just trying to balance yours and Gary’s >> >> points and find a good middle ground that everyone can live with. >> > >> > Which are the two sides for which you try to find a middle ground? >> > At any rate it would not be "good" if it makes developers and users >> > unhappy for a case that probably doesn't exist out there. >> > Tools such as Clirr are a great help, but they shouldn't induce us >> > to take the wrong course only to have the pleasure to contemplate a >> > clean report. ;-) >> > Thanks to Clirr (and Gary), we've become aware of a design issue. >> > We can decide to resolve it as it was done in the past, with IMHO >> > zero inconvenience (except for the Clirr "unclean" report). >> > >> > Regards, >> > Gilles >> > >> >> -Rob >> >> >> >>> >> >>> Gilles >> >>> >> >>>> -Rob >> >>>> >> >>>>> On Aug 9, 2018, at 7:02 AM, Gilles <gil...@harfang.homelinux.org> >> wrote: >> >>>>> >> >>>>> Hi all. >> >>>>> >> >>>>> On Wed, 08 Aug 2018 19:59:24 +0200, Gilles wrote: >> >>>>>> Hello Rob. >> >>>>>> >> >>>>>> On Wed, 8 Aug 2018 12:44:16 -0400, Rob Tompkins wrote: >> >>>>>>> @Gilles - thoughts here?? Just kinda what I was thinking, but I’m >> >>>>>>> only a +0 on this change. So, if you want to revert it before >> going up >> >>>>>>> with 1.1, that’s fine. >> >>>>>> >> >>>>>> I don't understand this change after what I answered to Gary's >> strange >> >>>>>> proposal. >> >>>>>> The comment is *wrong*. As I said previously, the base class >> provides >> >>>>>> boiler-plate code; that is *not* deprecated. The issue is that >> those >> >>>>>> methods were not meant to be used further down the hierarchy (in >> user's >> >>>>>> subclasses). [And now, furthermore, they are not used anymore in >> class >> >>>>>> "PoissonSampler", following the change in RNG-50 (delegation to >> other >> >>>>>> classes).] >> >>>>>> The sampler classes should have been made "final"; but now, this >> change >> >>>>>> would also be "breaking" (even though I doubt about legitimate >> use-cases >> >>>>>> for inherithing from the sampler implementations). >> >>>>> >> >>>>> Assuming it's unlikely that application developers would have >> >>>>> * called protected methods of "SamplerBase",[1] >> >>>>> * create subclasses of "SamplerBase",[1] >> >>>>> * create subclasses of the sampler classes,[1] >> >>>>> I suggest to >> >>>>> * make "SamplerBase" package private >> >>>>> * make the sampler clases "final". >> >>>>> >> >>>>> The above are the *less* intrusive fixes, as they would only >> >>>>> potentially impact application developers by making them aware >> >>>>> that they have made incorrect usage of an *inadvertently* >> >>>>> public API. Correct usage will not be impacted even though >> >>>>> the change is not BC.[2] >> >>>>> >> >>>>> Any objection to that fix?[3] >> >>>>> >> >>>>> Regards, >> >>>>> Gilles >> >>>>> >> >>>>> [1] Rationale: (a) There is no reason to do that and >> >>>>> (b) "Commons RNG" isn't much used at this point: >> >>>>> >> http://mvnrepository.com/artifact/org.apache.commons/commons-rng-sampling/usages >> >>>>> [2] I recall that a similar situation (BC breaking in a minor >> >>>>> release) occurred in CM, at a time where the number of >> >>>>> potential users was much larger. >> >>>>> [3] I'll add a prominent warning in the changelog to the effect >> >>>>> that people wanting to continue with incorrect usage of the >> >>>>> samplers should not upgrade. ;-) >> >>>>> >> >>>>>> >> >>>>>> Please revert. >> >>>>>> >> >>>>>> Thanks, >> >>>>>> Gilles >> >>>>>> >> >>>>>>> >> >>>>>>> -Rob >> >>>>>>> >> >>>>>>>> On Aug 8, 2018, at 12:42 PM, chtom...@apache.org wrote: >> >>>>>>>> >> >>>>>>>> Repository: commons-rng >> >>>>>>>> Updated Branches: >> >>>>>>>> refs/heads/1.1 50b984b1d -> f8159f28a >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> Adding PoissonSampler deprecations. Use the correct faster >> public methods >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> Project: http://git-wip-us.apache.org/repos/asf/commons-rng/repo >> >>>>>>>> Commit: >> http://git-wip-us.apache.org/repos/asf/commons-rng/commit/f8159f28 >> >>>>>>>> Tree: >> http://git-wip-us.apache.org/repos/asf/commons-rng/tree/f8159f28 >> >>>>>>>> Diff: >> http://git-wip-us.apache.org/repos/asf/commons-rng/diff/f8159f28 >> >>>>>>>> >> >>>>>>>> Branch: refs/heads/1.1 >> >>>>>>>> Commit: f8159f28a52197d0e7b55e39b115702147cf57a0 >> >>>>>>>> Parents: 50b984b >> >>>>>>>> Author: Rob Tompkins <chtom...@gmail.com> >> >>>>>>>> Authored: Wed Aug 8 12:42:45 2018 -0400 >> >>>>>>>> Committer: Rob Tompkins <chtom...@gmail.com> >> >>>>>>>> Committed: Wed Aug 8 12:42:45 2018 -0400 >> >>>>>>>> >> >>>>>>>> >> ---------------------------------------------------------------------- >> >>>>>>>> .../sampling/distribution/PoissonSampler.java | 42 >> ++++++++++++++++++++ >> >>>>>>>> 1 file changed, 42 insertions(+) >> >>>>>>>> >> ---------------------------------------------------------------------- >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> >> http://git-wip-us.apache.org/repos/asf/commons-rng/blob/f8159f28/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java >> >>>>>>>> >> ---------------------------------------------------------------------- >> >>>>>>>> diff --git >> a/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java >> b/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java >> >>>>>>>> index d0733ba..29d0e4e 100644 >> >>>>>>>> --- >> a/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java >> >>>>>>>> +++ >> b/commons-rng-sampling/src/main/java/org/apache/commons/rng/sampling/distribution/PoissonSampler.java >> >>>>>>>> @@ -67,6 +67,48 @@ public class PoissonSampler >> >>>>>>>> return poissonSampler.sample(); >> >>>>>>>> } >> >>>>>>>> >> >>>>>>>> + /** >> >>>>>>>> + * @return a random value from a uniform distribution in the >> >>>>>>>> + * interval {@code [0, 1)}. >> >>>>>>>> + * @deprecated - one should be using the {@link >> PoissonSampler#sample()} method, >> >>>>>>>> + * as it is considerably faster. >> >>>>>>>> + */ >> >>>>>>>> + @Deprecated >> >>>>>>>> + protected double nextDouble() { >> >>>>>>>> + return super.nextDouble(); >> >>>>>>>> + } >> >>>>>>>> + >> >>>>>>>> + /** >> >>>>>>>> + * @return a random {@code int} value. >> >>>>>>>> + * @deprecated - one should be using the {@link >> PoissonSampler#sample()} method, >> >>>>>>>> + * as it is considerably faster. >> >>>>>>>> + */ >> >>>>>>>> + @Deprecated >> >>>>>>>> + protected int nextInt() { >> >>>>>>>> + return super.nextInt(); >> >>>>>>>> + } >> >>>>>>>> + >> >>>>>>>> + /** >> >>>>>>>> + * @param max Upper bound (excluded). >> >>>>>>>> + * @return a random {@code int} value in the interval >> {@code [0, max)}. >> >>>>>>>> + * @deprecated - one should be using the {@link >> PoissonSampler#sample()} method, >> >>>>>>>> + * * as it is considerably faster. >> >>>>>>>> + */ >> >>>>>>>> + @Deprecated >> >>>>>>>> + protected int nextInt(int max) { >> >>>>>>>> + return super.nextInt(max); >> >>>>>>>> + } >> >>>>>>>> + >> >>>>>>>> + /** >> >>>>>>>> + * @return a random {@code long} value. >> >>>>>>>> + * @deprecated - one should be using the {@link >> PoissonSampler#sample()} method, >> >>>>>>>> + * * as it is considerably faster. >> >>>>>>>> + */ >> >>>>>>>> + @Deprecated >> >>>>>>>> + protected long nextLong() { >> >>>>>>>> + return super.nextLong(); >> >>>>>>>> + } >> >>>>>>>> + >> >>>>>>>> /** {@inheritDoc} */ >> >>>>>>>> @Override >> >>>>>>>> public String toString() { >> >>>>>>>> >> > >> > >> > --------------------------------------------------------------------- >> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> > For additional commands, e-mail: dev-h...@commons.apache.org >> > >> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >>
