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

Fix #1648 autocomplete with ts #1649

Merged
merged 5 commits into from
Jun 21, 2019
Merged

Fix #1648 autocomplete with ts #1649

merged 5 commits into from
Jun 21, 2019

Conversation

fdodino
Copy link
Collaborator

@fdodino fdodino commented May 26, 2019

Since there was a downcast to WMethodContainer sometimes we got a ClassCastException and autocomplete didn't work for variables or arguments.

This is the test with Type System activated
wollokIssue1648Solution

And with Type System deactivated
wollokIssue1648Solution2

@fdodino
Copy link
Collaborator Author

fdodino commented May 26, 2019

Oh crap! It seems I did a local commit while working on wdk folder deletion.
Consider it a minor detail, we can merge both PRs or close this PR and just merge this.

@coveralls
Copy link

coveralls commented May 26, 2019

Coverage Status

Coverage remained the same at 89.125% when pulling 446cdd9 on fix-#1648-autocomplete-with-ts into 3641243 on dev.

newArrayList
val untypedMethods = obj.allUntypedMethods
if (!obj.isTypeSystemEnabled) {
untypedMethods
Copy link
Member

Choose a reason for hiding this comment

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

No entiendo este cambio. El issue habla de un problema con el type system activado y la solución habla de un downcast. Pero el downcast en el código original ocure justamente cuando el ts está DESactivado. ¿De qué me perdí?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

El issue original ocurría con el type system activado.
Pero haciendo pruebas, me encontré con un ClassCastException cuando el type system no estaba activado, por ejemplo cuando quería trabajar esta expresión

const a = new Date()
a. // forzamos el autocomplete

@npasserini
Copy link
Member

npasserini commented May 27, 2019 via email

@fdodino
Copy link
Collaborator Author

fdodino commented May 27, 2019

Bueno, lo acabo de probar y funciona ok, así que ahora pusheo la versión sin synchronized (uno menos)

@PalumboN
Copy link
Contributor

Y sería muy costoso hacer que tire todos los mensajes posibles cuando no sabemos qué tipo es?

Copy link
Member

@npasserini npasserini left a comment

Choose a reason for hiding this comment

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

¿Mergeamos esto?

@fdodino
Copy link
Collaborator Author

fdodino commented Jun 21, 2019

Y sería muy costoso hacer que tire todos los mensajes posibles cuando no sabemos qué tipo es?

El tema es que cada mensaje depende del method container, habría que ver si a) es posible, b) se puede cachear porque me preocupa que le afecte la performance (si bien pagina el autocomplete, es una operación que demoraría un cierto tiempo). Si querés abrí un issue como propuesta y lo vemos.

@npasserini Y sí, totalmente, lo mergeamos así vamos liquidando temas pendientes...

@fdodino fdodino closed this Jun 21, 2019
@fdodino fdodino reopened this Jun 21, 2019
@fdodino
Copy link
Collaborator Author

fdodino commented Jun 21, 2019

Qué gil, me olvidé de mergear :^P

@fdodino fdodino merged commit 5120e33 into dev Jun 21, 2019
@fdodino fdodino deleted the fix-#1648-autocomplete-with-ts branch June 21, 2019 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants