[Refactorings] Fix for Negated condition violators#18597
[Refactorings] Fix for Negated condition violators#18597carolahp wants to merge 8 commits intopharo-project:Pharo14from
Conversation
Therefore violationMessageOn: makes sense even if a condition is negated.
|
Hey @carolahp, this looks interesting. Thanks for looking into this issue! I had some ideas, that I wanted to share. They go hand in hand with what you did, but some tweaks might be required. The core of it is: introduce |
|
Hi @balsa-sarenac thanks for your comments. You right about preventing nested wrappings of negations. To complement this idea and following what @Ducasse mentioned a few weeks ago, I made negated conditions return the condition when they receive the message |
| #instVars : [ | ||
| 'wrapper' | ||
| ], |
There was a problem hiding this comment.
this got left over, should be removed right?
| { #category : 'displaying' } | ||
| ReNewNegationCondition >> violationMessageOn: aWriteStream [ | ||
| | method re message message2 | | ||
| "If my condition is a negation, return its condition violation message" | ||
| condition class == self class | ||
| ifTrue: [ ^ condition condition violationMessageOn: aWriteStream ]. | ||
|
|
||
| "Otherwise, send the method defined in the condition to self. | ||
| In this way, violators return nonViolators" | ||
| method := condition class lookupSelector: #violationMessageOn:. | ||
| message := String streamContents: [:s | | ||
| method valueWithReceiver: self arguments: { s }]. | ||
|
|
||
| aWriteStream | ||
| nextPutAll: 'NOT '; | ||
| nextPutAll: condition errorString | ||
| "Remove 'not' apparitions from the message" | ||
| re := '[ ]+not[ ]*' asRegex. | ||
| message2 := re copy: message contents replacingMatchesWith: ' '. | ||
| message2 == message ifTrue: [ message2 := 'NOT ' + message ]. | ||
| aWriteStream nextPutAll: message2 | ||
| ] |
There was a problem hiding this comment.
This looks better, we should discuss it today
|
@carolahp @balsa-sarenac is is ok to merge this one? |
Not yet @Ducasse , I am currently fixing the failing tests |
|
@Ducasse I'll put |
|
Replaced by #18738 |
3 participants
Context
Condition violators are important for refactorings because they determine the actions taken by many drivers when breakingChangesPreconditions fail.
When violators are not accessible from drivers, they must replicate the logic behind preconditions to build descriptive error messages and implement the actions to handle possible failures.
Problem
Currently,
violatorsare not valid for NegatedConditions. Consider the following code:Solution
This PR is a proof of concept for a potential solution to this problem.
In this solution, a condition inside of a negation is aware of its corresponding negation condition, making it possible to delegate the result of the message
violatorsto the negation when necessary.The following code is an example, where the negation sets itself as the wrapper (wrapper is a bad name) of its condition and then tells it to print itself:
The condition delegates to the wrapper the responsibility to handle the return value of violators:
If the wrapper is nil, conditions return themselves, and therefore, they return violators in the traditional way:
But, if the wrapper is not nil (i.e. a negation), the violators message returns the condition's nonViolators:
Limitations
I am currently thinking of a more elegant solution because I don't like introducing an instance variable to Conditions