Comments (21)
@SentryMan Hey aren't you on vacation? Go back to enjoying that 😄
Also I owe it to you and @rbygrave to be tasked with things so let me know if you want me to do anything.
from avaje-config.
I'm good with it, but I'd let it sit in RC a little more in case we cook up more ideas.
from avaje-config.
If we're bumping a major might as well add that service loader change
from avaje-config.
What JDK version is avaje config compiled?
If 17 @SentryMan parent the class that is to be registered as the one and only SPI a sealed class like I did here: https://jstach.io/rainbowgum/io.jstach.rainbowgum/io/jstach/rainbowgum/spi/RainbowGumServiceProvider.html
EDIT I see JDK 11 as the min so I guess no sealed class.
from avaje-config.
We have got the ServiceLoader change in now so that completes all the tasks mentioned here. So now I think it's just a matter of confirming we are all good with 4.0-RC1
(released last night) and look to release 4.0
.
from avaje-config.
Also this:
Should throw either UncheckedIOException
or ignored (as in the file no longer exists).
EDIT that whole code block looks like duplication of stuff that Configuration.builder should do or know about:
InputStream resource(String resourcePath, InitialLoader.Source source) {
InputStream is = null;
if (source == InitialLoader.Source.RESOURCE) {
is = resourceStream(resourcePath);
if (is != null) {
loadedResources.add("resource:" + resourcePath);
}
} else {
File file = new File(resourcePath);
if (file.exists()) {
try {
is = new FileInputStream(file);
loadedResources.add("file:" + resourcePath);
loadedFiles.add(file);
} catch (FileNotFoundException e) {
throw new IllegalStateException(e);
}
}
}
return is;
}
Consequently when you use the Configuration.builder unlike the default loading:
The source information is missing in CoreEntry
and is the info is just initial
.
That is I expect:
var c = Configuration.builder().load("blah.properties").build();
assertEquals("resource:blah.properties",m c.entry("property_in_blah").orElseThrow().source());
But that is not the case. See #139
from avaje-config.
Not that it is super important but calling the ServiceLoader even just once is actually a significant cost.
That is a better solution is actually to have ResourceLoader
, ConfigurationLog
, ModificationEventRunner
all extend some parent interface and then make one call to the ServiceLoader
with the parent interface. Of course this gigantic breaking change for those that have implemented services already but just something to keep in mind for a major version change.
private static ResourceLoader initialiseResourceLoader() {
return ServiceLoader.load(ResourceLoader.class)
.findFirst()
.orElseGet(DefaultResourceLoader::new);
}
private ConfigurationLog initLog() {
if (configurationLog == null) {
configurationLog = ServiceLoader.load(ConfigurationLog.class)
.findFirst()
.orElseGet(DefaultConfigurationLog::new);
}
return configurationLog;
}
private ModificationEventRunner initRunner() {
if (eventRunner == null) {
eventRunner = ServiceLoader.load(ModificationEventRunner.class)
.findFirst()
.orElseGet(CoreConfiguration.ForegroundEventRunner::new);
}
return eventRunner;
}
from avaje-config.
ServiceLoader even just once is actually a significant cost.
Well now, I was actually thinking recently on how I could make things faster. It seems I've got my work cut out for me.
from avaje-config.
I can't specify a maybeLoad and or fallbacks.
It does now due to #143
I can't specify ... fallbacks.
I don't know what you mean by that. Maybe you are referring to ClassLoader.getSystemResourceAsStream.
ClassLoader.getSystemResourceAsStream
Yes that can be pushed down into the DefaultResourceLoader. Done now via #146
Should throw either UncheckedIOException
Done via #142
Consequently when you use the Configuration.builder unlike the default loading: The source information is missing in CoreEntry and is the info is just initial.
Ok, can have a look. Edit: Going with #147 for now, I'll have another look later.
from avaje-config.
Released as version 3.15-RC1, you can try that and hopefully that addresses most of the points above.
from avaje-config.
I don't know what you mean by that. Maybe you are referring to ClassLoader.getSystemResourceAsStream.
Oh what I mean is builder.load(File|Resource)
sort of implies that it will fail if it can't find it which is indeed what happened albeit a NPE.
Now I can see builder.load
is more like what I was calling a maybeLoad
as in if the file/resource is not there we just ignore trying to load it.
I think I'm fine with this but we should update the Configuration.Builder#load
javadoc to explicitly state that (if it doesn't find it an error will not happen).
In my ancient internal config library that we use in my company we have an enum of REQUIRE
and ALLOW_MISSING
or something. I think I really like how optionally loading is the only option because if it was really required like say in command line app you would probably do the checking yourself instead of delegating to avaje.
TLDR I like the changes just need some javadoc updates.
from avaje-config.
maybeLoad
An alternative would be to add a loadMaybe(...)
that takes resource and file and for the load(...)
methods to fail on missing resource.
This might be preferrable in that:
- It's probably more obvious for people using the builder
- It supports the case where people do want it to fail when a resource/file is missing
If we make that change, is loadMaybe(...)
a good enough and obvious enough name?
Sticking with the single load(...)
methods to me almost implies that 99% of the time missing resources are ok and that having both load(...)
and loadMaybe(...)
is less desirable (in a "less is more" kind of way).
Hmmm.
Edit: the opposite style is to add a loadRequired(resource|file)
that fails on missing resource
from avaje-config.
Hi @SentryMan , when you are back if you could just check if you are good with leaving this as a single load()
method with "loadMaybe" semantics (not throw an exception if the resource is missing).
If you are good with that and we don't want to change to add a loadMaybe()
or a loadRequires()
then we could probably release this next version.
from avaje-config.
Yeah cool. This RC has bug fixes in it though so I don't want to sit on it too long.
from avaje-config.
@rbygrave Sorry for the delay on the response. The only reason why I can see making
load() = loadRequire()
is because prior to the RC release it essentially was by accident 😆 .
So I don't know if that requires bumping a major for you... these days I have no idea what version policy folks are doing (I'm not sure if you saw my reddit post about semver).
I looked at our in house configuration library (one day I will just open source it so you and @SentryMan can laugh at the over engineering) and it appears we rarely do REQUIRE
.
The only time REQUIRE
API wise was used was to load defaults sort of similar to typesafe config reference.conf
but with an explicit name (typesafe in theory does not need a require because it loads all sources from the classpath or another words multiple reference.conf
).
However our config lib has a similar mechanism to load.properties
but where you could say it is required.
Honestly it added a whole bunch of complexity such that we created an enum called LoadFlag
and if we went down the path of making multiple load
given my experience I would recommend a similar pattern.
Just to give an idea of the disturbing level complexity you can get with LoadFlags:
public enum LoadFlag {
NOT_REQUIRED,
NOT_EMPTY,
NO_OVERRIDE,
FORCE,
NO_REPLACE_EXISTING_KEY_VALUE,
NO_ADD_KEY_VALUES,
STOP_ON_FIRST_FOUND,
NO_LOAD_CHILDREN,
NO_RESOURCE_PROVIDERS,
NO_DISCOVER_SERVICE_PROVIDERS,
NO_FINAL_INTERPOLATION,
NO_INTERPOLATION,
NO_LOGGING,
NO_RELOAD;
...
}
from avaje-config.
So I'm good with load
being optional.
The only thing I think that would seal the deal for me is to make load
doing some logging at say TRACE
on build
or InitialLoader
.
The reason is if one wants to figure out if a darn resource has loaded or not (see above we have NO_LOGGING
as it does it by default which is good and REQUIRED
was default when it should not have been).
from avaje-config.
That is excellent insight / feedback - thanks!!
... an idea of the disturbing level complexity you can get with LoadFlags
Yes, re-enforces the idea that we should try to keep it as simple as we can etc.
So I'm good with load being optional.
Great.
... load doing some logging at say TRACE on build or InitialLoader.
Lets add some logging there - I agree that is a real PITA when that is missing and people just want to know what was loaded.
requires bumping a major
I'm happy to bump the major for this. For me, I think this boils down to this change in behaviour of load()
being considered a breaking behaviour change and I think that is fair enough.
from avaje-config.
@SentryMan sorry for the sporadic comments (its post work and on and off). But I forgot to add the ServiceLoader
is like a memoized supplier. That is it is cached. Something to be aware of if you want to reduce resource loading.
from avaje-config.
@rob-bygrave I will put the change in version (RC) on rainbowgum to try it out today. Thanks!
from avaje-config.
Related Issues (20)
- Load configuration from a specific file - is it possible? HOT 14
- Extracting property source. HOT 6
- Use UncheckedIOException rather than IllegalStateException for consistency HOT 4
- Potential NPE in CoreConfigurationBuilder HOT 1
- ENH: Add Configuration.evalModify() ... to perform evaluation of expressions that modify properties in place
- Use AppLog.getLogger() rather than System.getLogger() to allow customization
- Remove the warning log messages when yml file extension is used rather than yaml
- Change so that properties via command line arguments is ALWAYS read (it wasn't when test resources detected)
- load.properties parameter should load from jar resources HOT 5
- loading from .localdev supports .yaml but not .yml (Add support for .yml with .localdev configuration)
- Maven Plugin Feature (and some other features). HOT 28
- SPI for initialization HOT 46
- Change from System.Logger to use events and control how those events are logged. HOT 2
- Event System does not coalesce changes properly HOT 8
- Document or fix hard dependency on avaje-inject when using module-path HOT 15
- Interpolation doesn't work in the same resource file.
- Multi-Profile support
- Make a Parser interface
- Support TOML? HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from avaje-config.