search

uqbar-project / wollok-ts

TypeScript based Wollok language implementation
GNU General Public License v3.0
20 stars 14 forks source link

Validación: Error cuando parámetro se llama igual a atributo #125

Open FerRomMu opened 2 years ago

FerRomMu commented 2 years ago

Error en consola: [ERROR]: parameterShouldNotDuplicateExistingVariable at packs.wlk:42

Link al repo

Detalle (el foco esta en la const cantidad y los metodos puedeGastarMB(cantidad) de las clases hijas):

class PackConsumible inherits Pack {

    const property cantidad
    const consumos = []

    method consumir(consumo) {
        consumos.add(consumo)
    }

    method cantidadConsumida() = consumos.sum({ consumo => consumo.cantidad() })

    method remanente() = cantidad - self.cantidadConsumida()

    override method acabado() = self.remanente() <= 0

}

class Credito inherits PackConsumible {

    override method cubre(consumo) = consumo.costo() <= self.remanente()

}

class MBsLibres inherits PackConsumible {

    override method cubre(consumo) = consumo.cubiertoPorInternet(self)

    method puedeGastarMB(cantidad) = cantidad <= self.remanente()

}

class MBsLibresPlus inherits MBsLibres {

    override method puedeGastarMB(cantidad) = super(cantidad) || cantidad < 0.1

}

Este error esta en coherencia con el wollok de eclipse que también lanzaba error si un atributo se llamaba igual que el parámetro (en eclipse solo va a lanzar ese error si el atributo se encuentra en la clase que tiene el método, si lo hereda como en este ejemplo no marca ni error ni warn). En tal caso, ¿no debería lanzar un warn y utilizar el parámetro dentro del método?

asanzo commented 2 years ago

Ok, lo que yo entiendo entonces es que esto anda MEJOR que el Wollok Xtext, ¿verdad? porque detecta el error de reusar el atributo aún si está en la superclase.

Mi pregunta es la misma que Fer: ¿No debería ser un warning? Es una discusión que podemos tener con más tiempo. Para mí esto en la planilla queda como verde: wollok-ts hace lo que se espera.

@FerRomMu ¿Te fijás si en wollok-language hay un test de esta validación, que incluya la validación en caso de heredar el atributo?

Si no la hay, cargate un issue en language para que la haya.

Por último, si querés podemos cerrar este issue y abrir otro con una "question" para discutir el warning level de esta validación.

FerRomMu commented 2 years ago

Acá el test (armé el correspondiente issue como pediste): Test atributo igual a parametro

Igual antes de cerrar estaría bueno ver esto que me sucedió!! Cuando lo fui a ejecutar con el test de cli me tiro error. Lo llamativo es que el error que lanza es el que el código espera parameterShouldNotDuplicateExistingVariable . Como si el wollokTS no estuviera pudiendo reconocer la etiqueta @Expect . Entiendo que si se pone esa etiqueta es para que si esta tal error el test pase verde y no se detenga su ejecución. No estoy seguro de que esto amerite otro issue.

lspigariol commented 2 years ago

me parece que es evidente que debe suceder lo mismo que sucede cuando el atributo está en la misma clase. si queremos cambiarle el nivel de warning sería una discusión futura.

El mar, 7 jun 2022 a la(s) 02:00, Fernando Romero @.***) escribió:

Acá el test (armé el correspondiente issue como pediste): Test atributo igual a parametro https://github.com/uqbar-project/wollok-language/blob/master/test/validations/parameterShouldNotDuplicateExistingVariable.wlk

Igual antes de cerrar estaría bueno ver esto que me sucedió!! Cuando lo fui a ejecutar con el test de cli me tiro error. Lo llamativo es que el error que lanza es el que el código espera parameterShouldNotDuplicateExistingVariable . Como si el wollokTS no estuviera pudiendo reconocer la etiqueta @Expect . Entiendo que si se pone esa etiqueta es para que si esta tal error el test pase verde y no se detenga su ejecución. No estoy seguro de que esto amerite otro issue.

— Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok-ts/issues/125#issuecomment-1148198488, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZRXGYRQ4N64JFJROVPCXLVN3JPHANCNFSM5X72X4TQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

FerRomMu commented 2 years ago

@nscarcella hay forma de correr en wollokTS los test de wollok lang que tienen la etiqueta @Expect ?

Como comente arriba si trato de correr el siguiente test falla:

class Person {
  var property toBeHidden = 23

  method repeatedParameters(
    @Expect(code="parameterShouldNotDuplicateExistingVariable", level="error")
    p1,
    @Expect(code="parameterShouldNotDuplicateExistingVariable", level="error")
    p1
  ) {}  

  method hidingFieldWithParameter(
    @Expect(code="parameterShouldNotDuplicateExistingVariable", level="error")
    toBeHidden
  ) {}

}

errorTest

nscarcella commented 2 years ago

Las annotations esas son solamente metadata (pensalas cómo comentarios formateados que quedan pegados a los nodos). No tienen una semántica propia y Wollok no opina sobre qué información puede contener o qué significa.

Hoy Wollok no tiene una forma de manipular o leer esa metadata así que, al menos por ahora, cada runner externo tiene que elegir contemplarla o no y cómo.

En particular, esas annotations se usan para el test de validaciones. Si mirás el runner del los tests de validations ( https://github.com/uqbar-project/wollok-ts/blob/master/test/validator.test.ts) vas a ver cómo. Pero si usas esa misma anotación en cualquier otro lado o corres los tests de validations con otro runner no van a hacer nada.

TL;DR: Si el runner que usas para correr los tests no las interpreta de alguna forma esas annotations son ignoradas.

El mié, 8 jun. 2022 12:18, Fernando Romero @.***> escribió:

@nscarcella https://github.com/nscarcella hay forma de correr en wollokTS los test de wollok lang que tienen la etiqueta @expect https://github.com/expect ?

Como comente arriba si trato de correr el siguiente test falla:

class Person { var property toBeHidden = 23

method repeatedParameters( @Expect(code="parameterShouldNotDuplicateExistingVariable", level="error") p1, @Expect(code="parameterShouldNotDuplicateExistingVariable", level="error") p1 ) {}

method hidingFieldWithParameter( @Expect(code="parameterShouldNotDuplicateExistingVariable", level="error") toBeHidden ) {}

}

[image: errorTest] https://user-images.githubusercontent.com/70177008/172652711-5ddd3356-20c0-4f6f-bfec-e8a14553b947.png

— Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok-ts/issues/125#issuecomment-1150058794, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFPE2ZGA63M5SMTJCDEOBLVOC2VRANCNFSM5X72X4TQ . You are receiving this because you were mentioned.Message ID: @.***>

asanzo commented 2 years ago

Gracias :heart:

Algunas preguntas:

nscarcella commented 2 years ago

El mié, 8 jun. 2022 14:44, asanzo @.***> escribió:

Gracias ❤️

Algunas preguntas:

  • Entonces si queremos correr los tests de validaciones que hay en wollok-language ¿lo que tenemos que hacer es clonarnos wollok-ts y correr los tests de validaciones? ¿ con npm run test:validations ?

Sí

  • ¿Por qué ese run test:validations no está dentro del script test y está separado?

Sí está dentro de test. Si corres npm test corre todo. Las corridas individual están por conveniencia.

  • ¿Cómo se relaciona wollok-ts con wollok-language? Veo que no está en las dependencies.

No es una dependencia exactamente. Language no tiene un package y no se buildea con npm. Al correr npm install en wollok-ts checkoutea la versión correspondiente de language y hace todo lo que tiene que hacer. Después podes encontrar la subcarpeta "language" con el repo checkouteado.

  • Y más específicamente, (porque esto es lo que queremos hacer a fin de cuentas) ¿Cómo agregamos un test de validaciones a wollok-language y lo corremos?

Language no es una implementación de Wollok, es una especificación. Ahí vos contás cómo se tendría que comportar una implementación correcta de Wollok, (por ejemplo qué cachos de código tendrían que hacer saltar qué validaciones) y ponés El código Wollok que toda implementación va a necesitar (cómo wollok.lang).

Eso permite que wollok-ts o cualquier otra implementación sepa si está funcionando bien.

En la teoría, Language no requiere ejecutar los test porque esos tests no pueden estar mal por definición: son la especificación que dice cuándo una implementación de Wollok está bien.

En la práctica uno se puede mandar cagadas escribiendo esos tests, entonces lo sano es probarlos contra una implementación confiable, con lo cual, lo típico es correrlos en wollok-ts.

El punto es que Language debería ser agnóstico de implementación. Eso permite discutir y definir features que todavía no tenemos implementado y dejar que las implementacionea corran atrás.

— Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok-ts/issues/125#issuecomment-1150211895, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFPE22IQPBLTH5PWT4LOE3VODLZRANCNFSM5X72X4TQ . You are receiving this because you were mentioned.Message ID: @.***>

asanzo commented 2 years ago

Ahhhh sí, ahí ví que test:unit corre sobre test/**/*.test.ts y eso incluye los tests de validations. Grax :+1:

Groso!! Sí, entiendo que wollok-language es único y se piensa como una "especificación" (y maso, porque en realidad tiene implementaciones bastante concretas). Pero entiendo el punto.

El tema es que wollok-language debería tener una especificación más: que no puedas usar como nombre de parámetro de un método un nombre de atributo que tenga tu superclase. Eso debería tirar error o warning. Esa especificación no está, y queremos agregarla a wollok-language por un tema de prolijidad.

Ahí ví fetchLanguage.ts. Y estuve mirando los scripts.

La carpeta language está dentro de la carpeta de wollok-ts así:

carpeta wollok-ts
     carpeta language (clonada automáticamente)

Entonces lo que hay que hacer para agregar un test de validación a wollok-language es:

¿estoy en lo correcto?

Gracias de nuevo :)

nscarcella commented 2 years ago

Sí, salvo que no hace falta correr buildWRE. Eso se corre solo cuando se instala language.

El mié, 8 jun. 2022 16:10, asanzo @.***> escribió:

Ahhhh sí, ahí ví que test:unit corre sobre test/*/.test.ts y eso incluye los tests de validations. Grax 👍

Groso!! Sí, entiendo que wollok-language es único y se piensa como una "especificación" (y maso, porque en realidad tiene implementaciones bastante concretas). Pero entiendo el punto.

El tema es que wollok-language debería tener una especificación más: que no puedas usar como nombre de parámetro de un método un nombre de atributo que tenga tu superclase. Eso debería tirar error o warning. Esa especificación no está, y queremos agregarla a wollok-language por un tema de prolijidad.

Ahí ví fetchLanguage.ts. Y estuve mirando los scripts.

La carpeta language está dentro de la carpeta de wollok-ts así:

carpeta wollok-ts

 carpeta language (clonada automáticamente)

Entonces lo que hay que hacer para agregar un test de validación a wollok-language es:

¿estoy en lo correcto?

Gracias de nuevo :)

— Reply to this email directly, view it on GitHub https://github.com/uqbar-project/wollok-ts/issues/125#issuecomment-1150294235, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFPE2Z65RYN2UXLDQEC5QTVODV2RANCNFSM5X72X4TQ . You are receiving this because you were mentioned.Message ID: @.***>