search

uqbar-project / wollok

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

In the REPL strings are shown without quotation marks #875

Closed gcrespi closed 8 years ago

fdodino commented 8 years ago

@npasserini @javierfernandes @tesonep Is it ok to add quotation marks in REPL responses? @flbulgarelli @gcrespi , Why was this issue reported with a Medium severity label? What are the consequences for students?

flbulgarelli commented 8 years ago

@fdodino the problem is that it introduces ambiguities that may not be so terrible within the context of the REPL, but that difficult debugging and understanding what is going on in more complex situations.

Side note: I think that the actual problem may not with the REPL itself, but the toString method, or a combination of both.

For example, the following three consults produce the same toString: 

>>> [true]
[true]
>>> ["true"]
[true]
>>> "[true]"
[true]

Which means that in a lot of cases the toString will lead into messages that are impossible to understand to a newcomer: 

>>> assert.equals([true], "[true]")
wollok.lang.Exception: Expected [[true]] but found [[true]]

Compare it to ruby's or haskell's REPLs: 

[9] pry(main)> [true]
=> [true]
[10] pry(main)> "[true]"
=> "[true]"
[11] pry(main)> ["true"]
=> ["true"]
Prelude> [True]
[True]
Prelude> ["True"]
["True"]
Prelude> "[True]"
"[True]"
Prelude>
fdodino commented 8 years ago

@flbulgarelli I don't know if a situation like 

>>> assert.equals([true], "[true]")

could arise in an initial course. My brain says "Ok, let's do it and see if tests pass ok" But my intuition says "Wait this year and see if the absence of quotation marks have consequences for students". Maybe I need to hear another opinions, @matifreyre @lgassman @asanzo (I can't find Lucas)

flbulgarelli commented 8 years ago

@fdodino actually they were my students who noticed that inconsistency during the first class, while using the repl and noticing that "1" and 1 produced the same result :stuck_out_tongue:

asanzo commented 8 years ago

Hi!

What are the implications of having:

"hola".toString() == "\"hola\""

instead of:

"hola".toString() == "hola"

?

Today I like Franco's reasoning, but I agree with Fer that it is not a major problem.

@fdodino, Do you see an additional reason why we should stay like we are?

Just thinking: Do we expect this to happen?

x.toString() == x.toString().toString()

I don't know. Right now I'd say as Homer: I like my beer cold, my tv loud, and my strings with quotation marks.

Lucas is @lspigariol

npasserini commented 8 years ago

Short story: I agree that strings should be shown with "" in REPL. But I am strongly against changing toString method at this stage, it requires a LOT more thinking. So, if you want a fast change, change the REPL, not the objects themselves.

Practical/empirical longer story: toString message migth be sent in many places, probably the toString of lots of objects depends so if you change it you will be putting " in lots of toStrings in undesired places, it will just not work. Or at least it is very dangerous. I am not sure that tests will cover it, it could introduce bugs, and we have an already started semester.

More philosophically, I already proposed that we need two (or even three) flavors of "toString" with different names:

Names could change, I do not care.

Also we could (or should!) discuss these categories. But I think that at least we should have two different "toString", and one of them should be the identity function on strings, because otherwise we loose polymorphism with the rest of the objects.

I would not change toString... at least I would not until we discuss this and we analyze the impact for each core object... and maybe after that I think that we could probably let toString as is and add another method for the new version.

So a fast way to add it would be to do it in the REPL. Or a nicer way could be to add printString, implement it as you proposed for toString and review which toStrings have to be changed for printString.

On Mon, Aug 22, 2016 at 5:34 AM, asanzo notifications@github.com wrote:

Hi!

What are the implications of having:

"hola".toString() == "\"hola\""

instead of:

"hola".toString() == "hola"

?

Today I like Franco's reasoning, but I agree with Fer that it is not a major problem.

@fdodino https://github.com/fdodino, Do you see an additional reason why we should stay like we are?

Just thinking: Do we expect this to happen?

x.toString() == x.toString().toString()

I don't know. Right now I'd say as Homer: I like my beer cold, my tv loud, and my strings with quotation marks.

Lucas is @lspigariol https://github.com/lspigariol

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/875#issuecomment-241308140, or mute the thread https://github.com/notifications/unsubscribe-auth/AEa1OeYYpc3sRamHrYa7hyTsoWwANncAks5qiRjCgaJpZM4JZj4z .

tesonep commented 8 years ago

+1 Nico, changing the Repl is the answer.

On 22 Aug 2016 14:21, "Nico Passerini" notifications@github.com wrote:

Short story: I agree that strings should be shown with "" in REPL. But I am strongly against changing toString method at this stage, it requires a LOT more thinking. So, if you want a fast change, change the REPL, not the objects themselves.

Practical/empirical longer story: toString message migth be sent in many places, probably the toString of lots of objects depends so if you change it you will be putting " in lots of toStrings in undesired places, it will just not work. Or at least it is very dangerous. I am not sure that tests will cover it, it could introduce bugs, and we have an already started semester.

More philosophically, I already proposed that we need two (or even three) flavors of "toString" with different names:

  • I think that toString has to be kept for more technical issues, internal debugging, should be very simple, guaranteed not to fail because it will be used by debugging and other internal tools.
  • printString provides a visual representation, for showing to the user... yes, an object could have many of these, but it is nice that we have at least one.
  • asString, converts an object to a String, and is just another member of many asXXX methods.

Names could change, I do not care.

Also we could (or should!) discuss these categories. But I think that at least we should have two different "toString", and one of them should be the identity function on strings, because otherwise we loose polymorphism with the rest of the objects.

I would not change toString... at least I would not until we discuss this and we analyze the impact for each core object... and maybe after that I think that we could probably let toString as is and add another method for the new version.

So a fast way to add it would be to do it in the REPL. Or a nicer way could be to add printString, implement it as you proposed for toString and review which toStrings have to be changed for printString.

On Mon, Aug 22, 2016 at 5:34 AM, asanzo notifications@github.com wrote:

Hi!

What are the implications of having:

"hola".toString() == "\"hola\""

instead of:

"hola".toString() == "hola"

?

Today I like Franco's reasoning, but I agree with Fer that it is not a major problem.

@fdodino https://github.com/fdodino, Do you see an additional reason why we should stay like we are?

Just thinking: Do we expect this to happen?

x.toString() == x.toString().toString()

I don't know. Right now I'd say as Homer: I like my beer cold, my tv loud, and my strings with quotation marks.

Lucas is @lspigariol https://github.com/lspigariol

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/875# issuecomment-241308140, or mute the thread https://github.com/notifications/unsubscribe-auth/ AEa1OeYYpc3sRamHrYa7hyTsoWwANncAks5qiRjCgaJpZM4JZj4z .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/875#issuecomment-241396093, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVsf58E0XwgYco3UIqxELTlpBFpCxPaks5qiZQvgaJpZM4JZj4z .

javierfernandes commented 8 years ago

I also would like to change the REPL and not the core objects. But I'm afraid that's not completely possible. That could work for simple objects/strings, but for lists for examples, the REPL will be calling "toString()" on the list and the logic to translate each object into strings is within Collection class, so the REPL won't be able to intercept that and show something like [ "a", "b ] instead of [ a, b ].

Unless the REPL starts to send another message like "print"

On Mon, Aug 22, 2016 at 9:24 AM, Pablo Tesone notifications@github.com wrote:

+1 Nico, changing the Repl is the answer.

On 22 Aug 2016 14:21, "Nico Passerini" notifications@github.com wrote:

Short story: I agree that strings should be shown with "" in REPL. But I am strongly against changing toString method at this stage, it requires a LOT more thinking. So, if you want a fast change, change the REPL, not the objects themselves.

Practical/empirical longer story: toString message migth be sent in many places, probably the toString of lots of objects depends so if you change it you will be putting " in lots of toStrings in undesired places, it will just not work. Or at least it is very dangerous. I am not sure that tests will cover it, it could introduce bugs, and we have an already started semester.

More philosophically, I already proposed that we need two (or even three) flavors of "toString" with different names:

  • I think that toString has to be kept for more technical issues, internal debugging, should be very simple, guaranteed not to fail because it will be used by debugging and other internal tools.
  • printString provides a visual representation, for showing to the user... yes, an object could have many of these, but it is nice that we have at least one.
  • asString, converts an object to a String, and is just another member of many asXXX methods.

Names could change, I do not care.

Also we could (or should!) discuss these categories. But I think that at least we should have two different "toString", and one of them should be the identity function on strings, because otherwise we loose polymorphism with the rest of the objects.

I would not change toString... at least I would not until we discuss this and we analyze the impact for each core object... and maybe after that I think that we could probably let toString as is and add another method for the new version.

So a fast way to add it would be to do it in the REPL. Or a nicer way could be to add printString, implement it as you proposed for toString and review which toStrings have to be changed for printString.

On Mon, Aug 22, 2016 at 5:34 AM, asanzo notifications@github.com wrote:

Hi!

What are the implications of having:

"hola".toString() == "\"hola\""

instead of:

"hola".toString() == "hola"

?

Today I like Franco's reasoning, but I agree with Fer that it is not a major problem.

@fdodino https://github.com/fdodino, Do you see an additional reason why we should stay like we are?

Just thinking: Do we expect this to happen?

x.toString() == x.toString().toString()

I don't know. Right now I'd say as Homer: I like my beer cold, my tv loud, and my strings with quotation marks.

Lucas is @lspigariol https://github.com/lspigariol

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/875# issuecomment-241308140, or mute the thread https://github.com/notifications/unsubscribe-auth/ AEa1OeYYpc3sRamHrYa7hyTsoWwANncAks5qiRjCgaJpZM4JZj4z .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/875# issuecomment-241396093, or mute the thread https://github.com/notifications/unsubscribe-auth/ AAVsf58E0XwgYco3UIqxELTlpBFpCxPaks5qiZQvgaJpZM4JZj4z .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/875#issuecomment-241396873, or mute the thread https://github.com/notifications/unsubscribe-auth/AEORWK7DrEx7y3cZpWpcEUj1nmofbUAIks5qiZUPgaJpZM4JZj4z .

fdodino commented 8 years ago

Ok, agree with Nico that changing toString() implementation for Strings scares me a lot. But if we just add a printString() method in Object that simply does self.toString(), overriden by String as toString() plus quotation marks, and if Repl console just asks for printString() method instead of toString(), that would do. Still if you do something like assert.equals([true], "[true]") you would get the error message wollok.lang.Exception: Expected [[true]] but found [[true]]. But I really don't think this is a good use of the language.

flbulgarelli commented 8 years ago

:+1: to adding a printString.

lspigariol commented 8 years ago

i like show "" in Repl and error messages too. agree with diferentiate print/show from convert/cast semantic

(i cant explain argumentations in issues comment nasty oficial language)

flbulgarelli commented 8 years ago

@lspigariol si se te complica comentá en español che :stuck_out_tongue:

fdodino commented 8 years ago

Well, thanks to Javi I've fixed in my branch the behavior of REPL console

>>> var saludo = "hola"
"hola"
>>> saludo == "hola"
true

And also fixed weird __synthetic0 and REPL lines of stack trace:

>>> "hola".concat(3)
wollok.lang.MessageNotUnderstoodException: hola does not understand concat(p0)
    at wollok.lang.Object.messageNotUnderstood(name,parameters) [/lang.wlk:212]

Obviously, printString produces several quotation marks:

>>> saludo.printString()
""hola""
npasserini commented 8 years ago

Why not call printString in the error message?

On Mon, Aug 22, 2016 at 6:52 PM, Fernando Dodino notifications@github.com wrote:

Ok, agree with Nico that changing toString() implementation for Strings scares me a lot. But if we just add a printString() method in Object that simply does self.toString(), overriden by String as toString() plus quotation marks, and if Repl console just asks for printString() method instead of toString(), that would do. Still if you do something like assert.equals([true], "[true]") you would get the error message wollok.lang.Exception: Expected [[true]] but found [[true]]. But I really don't think this is a good use of the language.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok/issues/875#issuecomment-241476732, or mute the thread https://github.com/notifications/unsubscribe-auth/AEa1ObiXrAQl5WyTroTAO1VeFGvbP0Lkks5qidPogaJpZM4JZj4z .

fdodino commented 8 years ago

Added printString for assert expectations

fdodino commented 8 years ago

Merged in REPL-fixes branch