Use UncheckedIOException rather than IllegalStateException for consistency

[agentgt]

In io.avaje.config.Config we have

public class Config {

  private static final Configuration data = CoreConfiguration.initialise();
...
}

Any static method called on Config will cause initialization of data. Luckily and I assume this is why @rbygrave wrote it that way is that every static method in Config happens to access data.

However another developer could easily make the mistake of adding a static method that does not need data. Perhaps a utility method used by the builder or whatever like concatPath or somethign.

Thus I recommend the canonical holder pattern of a private static inner class:

public class Config {

  private static class Holder {
    static final Configuration data = CoreConfiguration.initialise();
  }
...
}

All the static config methods would now do Holder.data for data.

[SentryMan]

I've no objections to this

[agentgt]

Also this:

https://github.com/avaje/avaje-config/blob/6915e81da2cad1927da0032eccc8740aafc7a0ac/avaje-config/src/main/java/io/avaje/config/InitialLoadContext.java#L102

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;
  }

EDIT whoops I put this in the wrong place

[rbygrave]

Holder ... However another developer could easily make the mistake of adding a static method that does not need data.

I'd say that is extremely unlikely. The design is such that Config has only the one job and I don't see that design changing. We get no real benefit from adding a Holder class per se. I don't think we need to make such a change.

Should throw either UncheckedIOException or ignored (as in the file no longer exists).

Well I don't think it should be ignored, I don't see a strong argument that ignoring that FileNotFoundException is the right thing to do.

It is certainly an unexpected state that a resource file exists and then doesn't when it is read. Throwing UncheckedIOException seems ok but IllegalStateException also seems ok given it has detected an unexpected and illegal state. I'll have a closer look there.

[rbygrave]

So yes lets change to use UncheckedIOException for better consistency.

I'll rename this issue to reflect this change. I don't see the desire for the Holder change but it you want to continue to push for that add some more comments here and lets see.