I don't have a strong feeling about null here (though we're Java 8 now - can't we start using Optional instead?). However it's always easier to add things and harder to remove things. If it's not really needed now, id vote to leave it out of this PR and add later if necessary.
On Thu, Feb 15, 2018, 11:47 PM Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > > 2018-02-15 20:00 GMT+01:00 Kenneth Knowles <k...@google.com>: > >> On Thu, Feb 15, 2018 at 12:03 AM, Romain Manni-Bucau < >> rmannibu...@gmail.com> wrote: >>> >>> 2. default properties = env + system properties: this is what all config >>> libs do (spring config, tamaya, deltaspike, microprofile-config, ...) so it >>> is very comfortable and sane for end users. It enables ops (env) and dev >>> (system properties) to feel comfortable for free with this and avoids us to >>> have N entry points which has the nice side effect to consolidate the >>> overall consistency of the framework/lib. >>> >> >> Can you link to the docs for these examples? I'm sure we'd like to learn >> from them. >> > > I'm not the best to find docs but the relevant bits are: > > > https://github.com/apache/deltaspike/blob/842c84bd1767905a19e2af01bdfe88d416d1b2da/deltaspike/core/impl/src/main/java/org/apache/deltaspike/core/impl/config/DefaultConfigSourceProvider.java#L57 > > https://github.com/apache/geronimo-config/blob/trunk/impl/src/main/java/org/apache/geronimo/config/DefaultConfigBuilder.java#L182 > > https://github.com/spring-projects/spring-framework/blob/a0bce618c2f51d8af1fc00ee2c3868ba2c8e0045/spring-core/src/main/java/org/springframework/core/env/StandardEnvironment.java#L78 > > (and there are others a bit less mainstream) > > The point is just that it is very common to read both and would be weird > to not have that feature if we read from system props. > > >> >> >>> 4. support null: I'd like to support null as a valid valid for >>> properties. The rational here is to make it neat for end user and support: >>> fromProperties(loadProps()) with a loadProps in the user codebase which can >>> return null. We just default to create(). For us it iss pretty much nothing >>> but it enables end users to not care about it. Concretely one null check >>> versus millions (or thousands at least ;)). >>> >> >> >> https://www.infoq.com/presentations/Null-References-The-Billion-Dollar-Mistake-Tony-Hoare >> > > :), concretely this applies only to your own codebase, as soon as you > start to expose an external API (intended pleonasm ;)) then you are forced > to handle null cause you can't help users to pass null. It doesn't imply > for all API in beam, for instance @ProcessElement can assume it is not null > in several cases but this fromXXX() is 100% delegated to the users. Here > the compromise is the cost of handling it once for all vs the cost to ask > all users to do it. My opinion is that it doesnt cost anything for beam, it > is safe for beam and therefore it is way better to put it in beam than ask > all users to care about it. > > That said I don't want to spend my week spare time speaking of that since > the feature is straight forward and not impacting the runtime. This is why > i droppped it from this PR. > > >> >> Kenn >> > >
