Provide an abstract base class to be able to test the formatter. #2966
Conversation
miklossy
force-pushed
the
miklossy_formatter_abstract_base_class
branch
from
63f6418
to
538de85
Compare
|
Just a comment about the CI failure: it looks like your PR is not based on the current master (it still uses Tycho 3 and old 4.31 builds. |
miklossy
force-pushed
the
miklossy_formatter_abstract_base_class
branch
from
538de85
to
087f315
Compare
|
Thanks for the hint @LorenzoBettini ! I have just rebased my feature branch on the master branch. Let's see if the CI bulid becomes green :-) |
|
Seems the build is green now. @LorenzoBettini : could you please review my changes? |
There was a problem hiding this comment.
I'm fine with this proposal, though I'd like to have confirmation also from @szarnekow
Just a few other notes: I seem to remember that FormatterTestHelper provides other configurations. Do you think it could be useful to have additional assert methods in the base class accordingly?
Moreover, I think it would be important to use this base class also in other parts, e.g., in our formatter tests (Xbase, Xtend?) so that we're sure we use it during the CI. In fact, the Domainmodel is not tested in the CI of Xtext.
We could also have separate issues for the above.
miklossy
force-pushed
the
miklossy_formatter_abstract_base_class
branch
from
087f315
to
a4d9485
Compare
|
Thanks for your feedback @LorenzoBettini !
I will request a review from @szarnekow as well.
I updated the PR so that the base class contains methods to configure the formatter preferences.
I updated the Xtend Formatter test cases to use this new API class. |
- Add the org.eclipse.xtext.testing.AbstractFormatterTest class. - Modify the FormatterTest class of the Domainmodel example to demonstrate the usage of the AbstractFormatterTest class. - Modify the AbstractXtendFormatterTest base class to inherit from the newly created AbstractFormatterTest class and adapt the Xtend formatter test cases to use the inherited members/methods. Signed-off-by: miklossy <miklossy@itemis.de>
miklossy
force-pushed
the
miklossy_formatter_abstract_base_class
branch
from
a4d9485
to
7fe793a
Compare
Thanks for the feedback, @LorenzoBettini ! |
@miklossy great! :) |
|
Thanks for the review @LorenzoBettini : do you know where are the release notes prepared for Xtext 2.35 so that I can add some hints about this new API class? |
Thank you for the contribution @miklossy . Concerning release notes, let's hear from @cdietrich |
|
please open a pr. @LorenzoBettini can you create stubs for the new release notes see also #2948 |
| }; | ||
| this._formatterTestHelper.assertFormatted(_function); | ||
| this.assertFormattedTo(_builder, _builder_1, _function); |
There was a problem hiding this comment.
@miklossy are you sure you committed all the generated files correctly? In my workspace, after using the new master with this PR merged, I get that this file should be
final Consumer<MapBasedPreferenceValues> _function = (MapBasedPreferenceValues it) -> {instead of the committed
final Procedure1<MapBasedPreferenceValues> _function = (MapBasedPreferenceValues it) -> {
There was a problem hiding this comment.
i see these changes too. can you open a fixing pr
concerning the changes applied here #2966 (comment)
concerning the changes applied here #2966 (comment)
@cdietrich done: eclipse/xtext-website#33 hope I got it right because that's the first time |
|
@miklossy the release notes files are in place eclipse/xtext-website#33 if you want to add some hints about the new classes |
|
Thank you @LorenzoBettini ! I will definitely do! |
|
@LorenzoBettini : I created the PR eclipse/xtext-website#34 about the hints. |
Closes #2965