Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Agregar validation para checkear que las consts dentro de un program esten inicializadas #175

Conversation

juanbono
Copy link
Member

@juanbono juanbono commented Oct 13, 2023

Arregla #151 .

El programa que use para testearlo es este y esta en este PR de wollok-language

program p {
    @Expect(code="shouldInitializeConst", level="error")
    const b
}

Creen que tambien deberia checkear constantes dentro de clases, objetos y tests?

@juanbono juanbono requested a review from PalumboN October 13, 2023 13:27
@juanbono juanbono changed the title add validation for uninitialized constants in programs Agregar validation para checkear que las consts dentro de un program esten inicializadas Oct 13, 2023
@fdodino
Copy link
Contributor

fdodino commented Oct 14, 2023

Hola @juanbono ,
buenísimo!! En cuanto a dónde agregar: sí en los objetos (Singleton) y en los tests. Es el comportamiento que hoy está en Wollok Xtext. No en las clases, dado que una clase puede definir valores en el momento de instanciarse: new Arbol(largo = 2)

@asanzo @nscarcella @PalumboN @ivojawer @npasserini

Copy link
Contributor

@PalumboN PalumboN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GROSSO @juanbono !!! 🚀

Ahí dejé un par de comments para que vayas tomando la onda.

Cosas que me pregunto (y para comentar):

  • Entiendo que para los atributos en objects y clases (y debería ser lo mismo con los mixines) está funcando como se espera (lo tocamos hace poco).
  • Lo que no sé qué pasa es dentro de los bloques... Habría que checkear.
  • Tal vez queremos redefinir la validación para que toda constante (no importa donde) venga inicializada. (Esto no afectaría al primer punto porque creo que los Module tienen Fields, no Variables. Pero sino bastaría con ignorar esos casos).
  • Hay que cargar un issue en el LSP para la traducción. (Para mí las traducciones deberían estar acá o en lanugage, pero no en el LSP. Qué decís @fdodino?)

src/validator.ts Outdated Show resolved Hide resolved
const isInProgram = getContainer(node)?.kind == 'Program'
return !(
isInProgram &&
!node.isAtPackageLevel &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Esto hace falta? Si isInProgram entonces no está al nivel package.

Comment on lines +448 to +450
node.value.isSynthetic &&
node.value.is(Literal) &&
node.value.isNull())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tal vez podamos abstraer esto en una función isNotInitialized, se repite en la otra validación.

@PalumboN
Copy link
Contributor

PalumboN commented Oct 18, 2023

Ahh otra cosa, si en vez de

Arregla #151 .

Ponés

Fix #151

Github ya linkea el issue con el PR y se cierra al mergear 😉

@PalumboN PalumboN linked an issue Oct 18, 2023 that may be closed by this pull request
@fdodino
Copy link
Contributor

fdodino commented Oct 20, 2023

Hay que cargar uqbar-project/wollok-lsp-ide#93. (Para mí las traducciones deberían estar acá o en lanugage, pero no en el LSP. Qué decís @fdodino?)

+1, eso fue una vieja discusión que perdí con @nscarcella y lo terminé implementando en wollok-lsp. Yo querría que tanto wollok-ts-cli como wollok-lsp-ide tengan internacionalización. Después wollok-ts y wollok-language pueden no tener en cuenta el idioma.

@PalumboN
Copy link
Contributor

Para mí que las validaciones devuelvan una etiqueta está bien, solo quiero los .json con las traducciones para consumirlo desde las otras apps :)

@juanbono
Copy link
Member Author

Buenisimo, muchas gracias a ambos @PalumboN @fdodino , mañana agrego los cambios pedidos 👍

@fdodino fdodino changed the base branch from master to update-ci October 30, 2023 03:02
@fdodino fdodino changed the base branch from update-ci to master October 30, 2023 03:03
@fdodino
Copy link
Contributor

fdodino commented Oct 30, 2023

@juanbono , hola! fijate cuando puedas

  1. si hacés las mejoras que te sugirió Nahue
  2. si te parece, hacete el PR directamente desde el repo en otro branch, porque este repo se tiene que bajar el wollok-language y es medio un bardo configurarle el secret key. Yo ya me fijé y sos member de uqbar-project, con lo cual no deberías tener problema, cualquier cosa avisame y te doy más acceesos.

image

Gracias!

@PalumboN
Copy link
Contributor

Recién veo que el CI no está corriendo, por qué hacen falta secret keys para correr los PRs @fdodino?

@fdodino
Copy link
Contributor

fdodino commented Nov 19, 2023

Recién veo que el CI no está corriendo, por qué hacen falta secret keys para correr los PRs @fdodino?

No estoy muy seguro, si pusheamos el mismo cambio desde una branch de este repo se romperá también? Ni idea por qué es necesario el secret key, porque wollok-language debería poder clonarse sin problemas ahora que lo pienso...

@fdodino
Copy link
Contributor

fdodino commented Nov 19, 2023

Recién veo que el CI no está corriendo, por qué hacen falta secret keys para correr los PRs @fdodino?

@PalumboN , el #188 lo creé desde el mismo repo sin problemas. Es medio una paja, hay que ver por qué necesitamos una key para bajar el repo de language (siendo que es un repo público). Cierro este PR y la seguimos en el otro mientras.

@fdodino fdodino closed this Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New validation: const without initialization
3 participants