uqbar-project / wollok

Wollok Programming Language
GNU General Public License v3.0
60 stars 16 forks source link

Should be a way to have an ordered list #510

Closed PalumboN closed 8 years ago

PalumboN commented 8 years ago

Sometimes the exercises in the guides require order a list by a condition. It would be great if Wollok could support it.

matifreyre commented 8 years ago

+1

javierfernandes commented 8 years ago

Ok there could be two different scenarios/requirements

  1. *to sort a list
  2. to have a sorted collection
npasserini commented 8 years ago

Thinking on most common teaching examples, I think that sorting a list covers pretty much all the exercises we usually use, and is more simple to teach than the other possibility.

On Sat, Dec 26, 2015 at 10:32 AM, javierfernandes notifications@github.com wrote:

Ok there could be two different scenarios/requirements 1. to sort a list 1. to have a *sorted collection

— Reply to this email directly or view it on GitHub https://github.com/uqbar-project/wollok/issues/510#issuecomment-167325172 .

matifreyre commented 8 years ago

You mean sorting the list with side-effect or create a new list with its elements sorted? I prefer the latter.

npasserini commented 8 years ago

I am not sure about what is best. Maybe you could reorder the same list and force a copy only if you need it.

On Wed, Dec 30, 2015 at 9:53 AM, Matías Freyre notifications@github.com wrote:

You mean sorting the list with side-effect or create a new list with its elements sorted? I prefer the latter.

— Reply to this email directly or view it on GitHub https://github.com/uqbar-project/wollok/issues/510#issuecomment-167991890 .

matifreyre commented 8 years ago

It's for didactic reasons, I prefer creating a brand new list. To begin with, it's the same you do in Smalltalk with asSortedCollection:, but it's also what you would do in Haskell and Prolog. Of course, you don't have the alternative of reordering the list there, but I feel like it's more consistent to do the same in Wollok. We can also have 2 different messages like sortBy(aClosure) and asSortedBy(aClosure).

flbulgarelli commented 8 years ago

For didactic purposes, a pure sort/sortBy is enough. Having an in-place sorting alternative is plus.

I didn't get the different between sort and asSorted, what was the intention of such selectors?

matifreyre commented 8 years ago

Reorder the same collection (side - effect) or create a new one (no side effect) respectively. El ene. 4, 2016 17:54, "Franco Leonardo Bulgarelli" < notifications@github.com> escribió:

For didactic purposes, a pure sort/sortBy is enough. Having an in-place sorting alternative is plus.

I didn't get the different between sort and asSorted, what was the intention?

— Reply to this email directly or view it on GitHub https://github.com/uqbar-project/wollok/issues/510#issuecomment-168805821 .

matifreyre commented 8 years ago

Any thought's on this? My proposal was to have 2 separate messages: sort: changes the receiving collection. asSorted: creates and returns a new collection. In both cases we could add a By at the end.

flbulgarelli commented 8 years ago

I like to have the four methods you mention. Just I am not conviced of the names, I think it is very related to #517 , because it depends on the decision about having a convention or not to mark imperative-style, effectful methods,

By the way, I think we can go with @matifreyre naming proposal and then rename if needed.

asanzo commented 8 years ago

I think the 4 method approach is elegant and suits every need. sort / asSorted / sortBy / asSortedBy You could teach using whichever you like.

matifreyre commented 8 years ago

I'm working on this. I'm having some problems with conversions between Java and Wollok objects, I just need some more time to find out where that conversion is being made.

javierfernandes commented 8 years ago

Look for references to this class

https://github.com/uqbar-project/wollok/blob/dev/org.uqbar.project.wollok/src/org/uqbar/project/wollok/interpreter/nativeobj/WollokJavaConversions.xtend

The piece of code that actually calls the native method (via reflection) is the one that performs the conversions in both directions -> java, and then the result back to wollok, using that other class WollokJavaConversions.

On Tue, Mar 8, 2016 at 10:15 AM, Matías Freyre notifications@github.com wrote:

I'm working on this. I'm having some problems with conversions between Java and Wollok objects, I just need some more time to find out where that conversion is being made.

— Reply to this email directly or view it on GitHub https://github.com/uqbar-project/wollok/issues/510#issuecomment-193780459 .

matifreyre commented 8 years ago

My problem was that I was returning this in a WList method, but it seems that a WList cannot be converted to WollokObject. WList extends WCollection, which in terms extends Collection. It's not related to WollokObject anyhow.

I was able to find a workaround to that by returning wrapped as List<WollokObject> instead, because List is already convertible. Is that a correct approach or should I add the conversion?

This is the complete method in WList.xtend (sortBy marked as native in wollok.wlk):

    def sortBy(WollokObject _predicate) {
        val closure = _predicate.asClosure
        /*Convert the closure into a comparator to use the standard sort*/
        val comparator = new Comparator {
            override def compare(Object a, Object b) {
                if(closure.doApply(a as WollokObject,b as WollokObject).isTrue)
                    return -1
                else
                    return 1
            } 
        }
        wrapped = wrapped.sortWith(comparator)
        //return this
        return wrapped as List<WollokObject>
    }
javierfernandes commented 8 years ago

Ok, so what you want is to return the same wollok object instance.

As you notice when coding native "this" in java is not the same as the "this wollok object" that you wanted semantically. So, your workaroud "works", but is not the best way to do it, as you said. Because for wollok it is like you are returning a new java list on each method execution. And what it will do is to create a new wollok list wrapping that list. So, on the wollok side you'll see that it returned a new object (instance) even if at the end it has the same elements.

To accomplis what you want (return the same wollok object), you need to have access to the WollokObject from your native code. To do that, you can declare a constructor with the following form:

new(WollokObject instance) { this.instance = instance }

and then return it

def sortBy(..) { ... return instance }

Wollok will notice that it is a WollokObject so no need for conversions. And it will be the same instance.

I hope this helps

Regards

PD1: It was not necessary to case "as List<...>" in your example (I think so) PD2: There's more info on the native objects constructor conventions here https://github.com/uqbar-project/wollok/wiki/LanguageReference#natives "Access to Wollok Object section" PD3: There's even more insights on the inner desing of the interpreter here https://github.com/uqbar-project/wollok/wiki/Development---Design---Natives-Mechanism-Implementation PD4: Development documentation of the interpreter https://github.com/uqbar-project/wollok/wiki/Development#wollok-design

On Tue, Mar 8, 2016 at 11:04 AM, Matías Freyre notifications@github.com wrote:

My problem was that I was returning this in a WList method, but it seems that a WList cannot be converted to WollokObject. WList extends WCollection, which in terms extends Collection. It's not related to WollokObject anyhow.

I was able to find a workaround to that by returning wrapped as List instead, because List is already convertible. Is that a correct approach or should I add the conversion?

This is the complete method in WList.xtend (sortBy marked as native in wollok.wlk):

def sortBy(WollokObject _predicate) {
    val closure = _predicate.asClosure
    /*Convert the closure into a comparator to use the standard sort*/
    val comparator = new Comparator {
        override def compare(Object a, Object b) {
            if(closure.doApply(a as WollokObject,b as WollokObject).isTrue)
                return -1
            else
                return 1
        }
    }
    wrapped = wrapped.sortWith(comparator)
    //return this
    return wrapped as List<WollokObject>
}

— Reply to this email directly or view it on GitHub https://github.com/uqbar-project/wollok/issues/510#issuecomment-193795952 .

matifreyre commented 8 years ago

The constructor made it work like a charm :-) I need to add some tests and that would be all.

About PD1: It wasn't really needed, but avoided a warning due to the inferred return type of the method.

javierfernandes commented 8 years ago

Cool !

On Tue, Mar 8, 2016 at 12:14 PM, Matías Freyre notifications@github.com wrote:

The constructor made it work like a charm :-) I need to add some tests and that would be all.

About PD1: It wasn't really needed, but avoided a warning due to the inferred return type of the method.

— Reply to this email directly or view it on GitHub https://github.com/uqbar-project/wollok/issues/510#issuecomment-193819061 .