refactor: simplify implementation of getCaseExpression and setCaseExpression

[algomaster99]

Fixes #4019

These changes removed the this.caseExpression attribute as it is obsolete since the arrival of this.caseExpressions. The latter is enough to handle one or more case expressions for a single case.

Before merging, the following tasks need to be done.

• Fix the failing test case - APITest.testSetterInNodes.
• Write tests for the new implementation.

[algomaster99]

The test fails here in the CI. It expects one if condition in setCaseExpression but it gets none as asserted here. I think the modified implementation is unable to match the template and that's why no if condition is found in setCaseExpression even though there is one. I need some help fixing this because the other setters in CtCaseImpl are written in a similar format but they pass the test. I am not sure what is going wrong for this single setter.

[algomaster99]

@monperrus @slarse I was also wondering if writing contract as a comment is necessary here as I have already written it using the DisplayName annotation. I could write it for the sake of consistency so let me know.

[algomaster99]

Ping @monperrus @slarse for reviews.

[slarse]

Implementation LGTM. Some minor comments on the tests.

Also, about the display names, I would put the method under test into them as the first thing written, because that's the name seen when running the tests. And so it's nice to know which method is under test just by looking at the name. Since the other nested types in the class are not categorized by method, it's not sufficient to rely on the name of the nested type for this one case.

[slarse]

@monperrus @slarse I was also wondering if writing contract as a comment is necessary here as I have already written it using the DisplayName annotation. I could write it for the sake of consistency so let me know.

If the contract is adequately expressed in the display name, then no that's not necessary. But it should be evident what part/piece of funcionality is being tested, and under what circumstances, and what the expected result is.

[algomaster99]

I would put the method under test into them as the first thing written,

How about I omit the DisplayName annotation so that the method name only displays on the side panel? My reason for doing so is that prepending the method name would make the display name quite long and one would have to expand the IntelliJ side panel by a lot to read the otherwise truncated words. However, it may be more readable when one runs the test using the terminal.

I propose that we omit the DisplayName annotation and write it as a contract using inline comments.

[slarse]

This looks good to me now, but I'd like @monperrus to have a look as well since this is (technically speaking) a metamodel change. Although internal.

@monperrus Please see what you think of this, it looks like a good patch to me.

[monperrus]

I like this simplification (simpler is better: this is purely internal, the API remains the same, and all tests are green.

👍

Thanks a lot @algomaster99

[slarse]

Thansk @algomaster99, nicely done!