"Props.get" improvement. Pull-request. Question on Box methods.

[Vasya Novikov]

I noticed a strange behavior of Props object: if you try to open some
property by name and it didn't exist, Props.get returns an Empty box
with no reasoning about the error.

But, if you add a simple error reason, things will become more logical
and logging starts to be better. For example, you will get this if you
open your box using `.openOrThrowException("")`:

Cause: net.liftweb.common.Failure$$anon$1: An Failure Box was opened.
Failure Message: could not load configuration property 'my.test'. The
justification for allowing the openOrThrowException was
[info] at net.liftweb.common.Failure.openOrThrowException(Box.scala:757)
[info] at net.liftweb.common.Failure.openOrThrowException(Box.scala:726)
...


I made a PR to fix things (as I think, at least)
https://github.com/vn971/framework/commit/7d91899c8957026b803aeeb3938ef5696b2d522c

Are there any additional thoughts/doubts? Lift contribution guidelines
ask to create an ML topic before PR. I also can cherry-pick this commit
onto any other branch (if needed).


Also, a separate question. Will you accept a PR that _deletes_ all
method bodies from abstract class "Box"? Currently, Box declares _and
implements_ lots of methods. Some of them are overridden by descendant
classes.
The problem with that approach is, as I see it, readability. Currently,
if you open a method on Box, you'll see something like:
`
def map[B](f: A => B): Box[B] = Empty
`
My first reaction to this is always "WTF do I need that stupid method
for?". Ok, after some time I of course remember about the descendants an
object-orientation of the code.. Still, I think having no implementation
in Box itself would be cleaner. Thoughts?




--
Vasya Novikov

[Matt Farmer]

Hi there,

First thanks for your contributions. I’ll address your points one by one.

I made a PR to fix things (as I think, at least)
https://github.com/vn971/framework/commit/7d91899c8957026b803aeeb3938ef5696b2d522c

This behavior isn’t desirable.

Properties may or may not be defined, and from the point of view of the framework, an undefined property should indeed yield an Empty. That’s what the Empty means semantically. There was nothing. If that’s a failure condition for you code, I recommend following your Props.get invocation with ?~ or failMsg (they do the same thing). Here’s a good example of doing that:

https://github.com/farmdawgnation/wepay-scala/blob/master/src/main/scala/me/frmr/wepay/WePay.scala#L60-L71

As you can see, there are two examples here that I used. The first group of them are similar to yours. If you try to access that parameter you get a very specific failure message explaining the setting that was missing. But in other cases, the property is just an override of something that I have a default value for internally.

The framework can’t assume that the absence of a property is, in fact, a failure condition - only the application can do that.

Also, a separate question. Will you accept a PR that _deletes_ all
method bodies from abstract class "Box"? Currently, Box declares _and
implements_ lots of methods. Some of them are overridden by descendant
classes.

This would result in pushing all of that default implementation into the implementing classes. At first blush that strikes me as more error prone. It’s not a change I, personally, am interested in seeing right now, but other committers may disagree.

Thanks,

—

Matt Farmer | Blog | Twitter
GPG: CD57 2E26 F60C 0A61 E6D8  FC72 4493 8917 D667 4D07

--
--
Lift, the simply functional web framework: http://liftweb.net
Code: http://github.com/lift
Discussion: http://groups.google.com/group/liftweb
Stuck? Help us help you: https://www.assembla.com/wiki/show/liftweb/Posting_example_code

---
You received this message because you are subscribed to the Google Groups "Lift" group.
To unsubscribe from this group and stop receiving emails from it, send an email to liftweb+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.