Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Make Lift build for scala 2.10! #1393

Merged
merged 7 commits into from Jan 8, 2013

Conversation

Projects
None yet
5 participants
Contributor

nafg commented Jan 7, 2013

No description provided.

would you mind explaining if this additional code fixed an error or what is the purpose of it? I'm not questioning that we need it, but it is more along the lines I have no idea what this does :)

Contributor

nafg replied Jan 7, 2013

Yes, 2.10 is much more allergic to recursive "MyType." That is, it needs more strict proof; you can't use something that the compiler doesn't know is precisely that type. The above encodes the general "recursive Mapper MyType" so we can use it when the type is not fully known. See for instance line 40 below. I was really stuck on it, actually originally in Record, and this was suggested by Aleksey Nikiforov here: https://groups.google.com/d/topic/scala-language/bPWlyonbODI/discussion

Owner

fmpwizard commented on d175b71 Jan 7, 2013

+1

Owner

fmpwizard commented on 985a975 Jan 7, 2013

+1

Owner

davewhittaker commented on a137d7a Jan 7, 2013

I'm guessing that Scala 2.10 was having some type of issue with the wildcard type that necessitated the funky typing you added. Care to explain the details of that? Maybe it's the only way to get the typing right, but introducing a null value just to extract it's inner type.... well, it's certainly not pretty.

Contributor

nafg replied Jan 7, 2013

Heh, Diego just asked about that where mapper needed it. See my comment there: d175b71#persistence-mapper-src-main-scala-net-liftweb-mapper-onetomany-scala-P7

Contributor

nafg replied Jan 7, 2013

Perhaps we should put it somewhere that it's more generally available, instead of the repetition. Personally I don't mind not dong so though. :)

@Shadowfiend Shadowfiend commented on the diff Jan 7, 2013

core/json/src/test/scala/net/liftweb/json/XmlBugs.scala
@@ -52,13 +52,15 @@ object XmlBugs extends Specification {
<n id="10" x="abc" />
<n id="11" x="bcd" />
</root>
- val expected = """{"root":{"n":[{"x":"abc","id":"10"},{"x":"bcd","id":"11"}]}}"""
- Printer.compact(render(toJson(xml))) mustEqual expected
+ val expected = """{"root":{"n":[{"x":"abc","id":"10"},{"x":"bcd","id":"11"}]}}"""
+ val expected210 = """{"root":{"n":[{"id":"10","x":"abc"},{"id":"11","x":"bcd"}]}}"""
@Shadowfiend

Shadowfiend Jan 7, 2013

Owner

Uh-oh. Why are these backwards? Order can sometimes matter in these (for example, I believe in MongoDB the incoming field order can matter, though lift-mongo may avoid this serialization step and use its own), and reversing the emitted field order could be a problem…

@nafg

nafg Jan 7, 2013

Contributor

The question is why scala.xml in 2.9 did not preserve the attributes' order.

@Shadowfiend

Shadowfiend Jan 7, 2013

Owner

Hah. That's what I get for not paying enough attention to context. Sorry, I thought this was the JValue-to-String serialization. Sounds good.

@nafg

nafg Jan 7, 2013

Contributor
~/dev/liftweb/framework (nafg_wip_scala210 ✘)✭ ᐅ scala                                                                                                                        127 ↵
Welcome to Scala version 2.9.2 (Java HotSpot(TM) 64-Bit Server VM, Java 1.7.0_10).
Type in expressions to have them evaluated.
Type :help for more information.

scala> val xml =
     |       <root>
     |         <n id="10" x="abc" />
     |         <n id="11" x="bcd" />
     |       </root>
xml: scala.xml.Elem = 
<root>
        <n x="abc" id="10"></n>
        <n x="bcd" id="11"></n>
      </root>

@Shadowfiend Shadowfiend commented on the diff Jan 7, 2013

...rc/main/scala/net/liftweb/mapper/view/ModelView.scala
@@ -64,7 +64,7 @@ trait ModelSnippet[T <: Mapper[T]] extends StatefulSnippet {
def load(entity: T) = view.entity = entity
- def dispatch = {
+ def dispatch: DispatchIt = {
@Shadowfiend

Shadowfiend Jan 7, 2013

Owner

Is the inferencer no longer able to do these for us? Will this percolate down into Lift users as well?

@nafg

nafg Jan 7, 2013

Contributor

It didn't compile, so I fear it other code bases will have to make the change too. I don't know of a solution though. 2.10 is just more strict.
The truth is one of those involved told me later that anything that breaks in 2.10 should be filed as a (possible) bug, but I didn't know that. Too late now...

@Shadowfiend

Shadowfiend Jan 7, 2013

Owner

Damn. It seems like we should file a bug anyway, and just leave this declaration here. That way even though we have to make this change, hopefully others won't.

@nafg

nafg Jan 7, 2013

Contributor

2.10 is released, there's no chance other's won't have to, unless it's fixed in 2.10.1 and people wait for it (although it is supposed to be out in only 4 months).

@Shadowfiend

Shadowfiend Jan 7, 2013

Owner

Yeah, that's what I meant. For those who wait for a .1 or later point release, they could avoid making this change. I don't think that's a blocker on this PR.

@Shadowfiend Shadowfiend commented on the diff Jan 7, 2013

...c/main/scala/net/liftweb/record/field/EnumField.scala
@@ -70,7 +70,7 @@ trait EnumTypedField[EnumType <: Enumeration] extends TypedField[EnumType#Value]
* is the value of the field and the second string is the Text name of the Value.
*/
def buildDisplayList: List[(Box[EnumType#Value], String)] = {
- val options = enum.values.map(a => (Full(a), a.toString)).toList
+ val options = enum.values.toList.map(a => (Full(a), a.toString))
@Shadowfiend

Shadowfiend Jan 7, 2013

Owner

Is this a preference thing or are you addressing a problem in 2.10? If so, what's the problem?

@nafg

nafg Jan 7, 2013

Contributor

Enumeration.ValueSet is not ordered. I don't know if it worked on 2.9 reliably. In general, I don't think I made any changes that were not to get a source to compile or a test to pass.

@nafg

nafg Jan 7, 2013

Contributor

To be more precise, ValueSet is a SortedSet, with an Ordering based on the values' id, so toList works correctly. However map will yield a Set[(Box[Value], String)], which will not preserve the order.

@Shadowfiend

Shadowfiend Jan 7, 2013

Owner

Ok, cool. That's annoying, but I can see why it is that way.

Owner

Shadowfiend commented Jan 7, 2013

👍 ;)

Owner

fmpwizard commented on 37c7256 Jan 8, 2013

+1

do you know if we'll get scalaz 6 build for 2.10 final?

Contributor

nafg replied Jan 8, 2013

Oh they actually did release it since. Let me update it.

This could probably stand to be updated - I'm including lift-json-scalaz 2.5-RC2 on Scala 2.9.2 right now, and it's downloading scalaz-core_2.9.1, instead of just using the one I'm already using - http://search.maven.org/#artifactdetails%7Corg.scalaz%7Cscalaz-core_2.9.2%7C6.0.4%7Cjar

Owner

fmpwizard replied Mar 15, 2013

Hi, could you open a ticket and then email the lift mailing list? I'll take care of it before we do the next Lift build. Thanks

did you run into issues where making this changed caused a test to fail? I remember trying to remove this warning (on 2.9.x), but then a test would fail.

Contributor

nafg replied Jan 8, 2013

Rings a bell, I don't remember how it was fixed.

Owner

fmpwizard commented Jan 8, 2013

Looks great, thanks for the work to get the code in line with scala 2.10!

@nafg nafg merged commit a137d7a into master Jan 8, 2013

@nafg nafg deleted the nafg_wip_scala210 branch Jan 8, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment