Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

introduced a ValueError that represents an error caused by a config value that isn't the expected type #43

Closed

Conversation

ppurang
Copy link
Contributor

@ppurang ppurang commented Nov 2, 2016

Ideally the error message should also carry the information of the type expected. But that would mean somehow accessing type information on any 'A'.

@stew
Copy link
Contributor

stew commented Nov 4, 2016

@ppurang This looks like a nice change, so thanks!

The one concern I have is that this introduces an incompatibility with earlier versions, since you are introducing a new Exception that could be thrown.

Imagine that someone has code:

try {
  cfg.require[String]("test.immutable.value.error")).
} catch {
  case KeyError(name) => 
     // express a default
}

They now have to introduce a new exception handler to transform the same set of inputs identically.

So I like this change, but I think that we need to introduce a version bump to 3.11.x with it.

@ppurang
Copy link
Contributor Author

ppurang commented Nov 4, 2016

@stew I agree with you - this is an incompatibility eventhough we never explicitly define the exceptions we raise.

I hope everyone does something more appropriate to make the API less "exceptional" like:

Maybe.fromTryCatchThrowable(
                cfg.require[String]("test.immutable.value.error")
           )
           .getOrElse("default")

Another thing bothers me even more. The previous PR that was accepted and has been released might break dependent code even more. Because of an issue we were pulling in all system properties given a prefix. Example:

//before the present version

//all system properties become part of cfg
val cfg = load(List(Required(SysPropsResource(Prefix("user")))))  
val path = cfg.require[String]("path.separator")  // found 

//present version

//only system properties begining with 'user' become part of cfg
val cfg = loadConfig(Required(SystemProperties(Prefix("user"))))
val path = cfg.require[String]("path.separator")  // not found! 

Thanks for looking into the PR.

@ppurang ppurang mentioned this pull request Jan 20, 2017
@rossabaker rossabaker mentioned this pull request Feb 23, 2017
@timperrett timperrett closed this Feb 24, 2017
@timperrett
Copy link
Contributor

Included in #51

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants