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 #1486 if with assignment #1541

Merged
merged 11 commits into from
Dec 7, 2018
Merged

Fix #1486 if with assignment #1541

merged 11 commits into from
Dec 7, 2018

Conversation

fdodino
Copy link
Collaborator

@fdodino fdodino commented Oct 16, 2018

Este tiene

  • el mensaje customizado para cuando hay una asignación en lugar de un == y te sugiere que lo cambies
  • arreglé overriden => overridden que estaba diseminado en varios lugares

@fdodino fdodino added this to the Wollok v1.7.2 milestone Oct 16, 2018
@ghost ghost assigned fdodino Oct 16, 2018
@ghost ghost added the in progress label Oct 16, 2018
@npasserini
Copy link
Member

npasserini commented Oct 16, 2018 via email

@fdodino
Copy link
Collaborator Author

fdodino commented Oct 16, 2018

No lo hice, tendría que probarlo bien

@fdodino
Copy link
Collaborator Author

fdodino commented Oct 22, 2018

ahí lo armé con test del quick fix y todo:

anim

@coveralls
Copy link

coveralls commented Oct 22, 2018

Coverage Status

Coverage decreased (-0.005%) to 90.174% when pulling cc37c30 on fix-#1486-if-with-assignment into 5005987 on dev.

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.

Dejé unos comentarios usando el feature "suggestion" que dijo github... supuestamente se pueden aplicar directamente, pero no sé bien como. Me pareció intersante el feature para acelerar el proceso de revisión, veamos si cumple con lo que promete.

@@ -820,7 +821,11 @@ class WollokDslValidator extends AbstractConfigurableDslValidator {
@CheckGroup(WollokCheckGroup.POTENTIAL_PROGRAMMING_PROBLEM)
def nonBooleanValueInIfCondition(WIfExpression it) {
if (!condition.isBooleanOrUnknownType) {
report(WollokDslValidator_EXPECTING_BOOLEAN, it, WIF_EXPRESSION__CONDITION)
if (condition instanceof WAssignment) {
Copy link
Member

Choose a reason for hiding this comment

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

Esto sólo chequea que la asignación esté en la raiz del nodo condición, hay otras posibilidades que no estamos considerando.

  • En este mismo punto, si hubiera un árbol de condicionales (tipo A and B or C), sería interesante hacer la misma validación sobre sus componentes.
  • En otros lugares donde aparezca el = en un lugar donde se espera una expresión (como argumento en un mensaje, return, etc).

No pararía el PR por esto, pero tal vez podríamos registrar esos issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahí le agregué validaciones a la condición binaria.
En el return el mensaje que tira es "Cannot return an assignment".
Me parece bien continuarlo como issues donde evaluemos qué tipo de mensaje salta y cómo lo queremos tunear.

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.

👏

@fdodino fdodino merged commit 6efeca5 into dev Dec 7, 2018
@fdodino fdodino deleted the fix-#1486-if-with-assignment branch December 7, 2018 11:48
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.

None yet

3 participants