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

Interactive mr tree #145

Merged
merged 30 commits into from
Mar 14, 2024
Merged

Interactive mr tree #145

merged 30 commits into from
Mar 14, 2024

Conversation

soerendomroes
Copy link
Member

No description provided.

@soerendomroes soerendomroes marked this pull request as ready for review March 7, 2024 14:39
@soerendomroes
Copy link
Member Author

@NiklasRentzCAU do you know the error message?

@soerendomroes
Copy link
Member Author

Somehow, moving the constraint serializer resulted in no constraints being serialized since the LSP client does not seem to be the correct one.

@NiklasRentzCAU
Copy link
Member

@NiklasRentzCAU do you know the error message?

Yes - publishing to Maven Central required us to also check in valid JavaDoc. The check now also goes through the Javadoc, and alongside quite a few warnings for missing JavaDoc there are four errors in your new JavaDoc, see the non-capitalized "error: ..." markings in the actions message: InteractiveUtil has a @returns tag (should be @return), MrTreeInteractiveLanguageServerExtension, PositionConstraintReevaluation, and RectpackingInteractiveLanguageServerExtension refer to variables with @param that do not exist, probably because they have been renamed.

@soerendomroes
Copy link
Member Author

Yes - publishing to Maven Central required us to also check in valid JavaDoc. The check now also goes through the Javadoc, and alongside quite a few warnings for missing JavaDoc there are four errors in your new JavaDoc, see the non-capitalized "error: ..." markings in the actions message: InteractiveUtil has a @returns tag (should be @return), MrTreeInteractiveLanguageServerExtension, PositionConstraintReevaluation, and RectpackingInteractiveLanguageServerExtension refer to variables with @param that do not exist, probably because they have been renamed.

There are a lot of error messages to begin with on the master. Did we just not fix all the issues on main before activating this? @NiklasRentzCAU

@soerendomroes
Copy link
Member Author

If we enforce this, do we have a checkstyle configuration for this?

@soerendomroes
Copy link
Member Author

This enforces comments for Data classes such as CheckedImagesAction and ConstraintProperty. How do I even add a comment for a data class constructor? I do not think that the current configuration is reasonable.

@soerendomroes
Copy link
Member Author

My bad, the rest are Errors and warnings.

@soerendomroes soerendomroes requested review from NiklasRentzCAU and removed request for NiklasRentzCAU March 12, 2024 10:18
@soerendomroes
Copy link
Member Author

@NiklasRentzCAU the last commit creates and uses the IActionHandler service interface properly for the interactive layout handlers. I plan to use this infrastructure for the structure-based-editing handler too. Please have a look at this and ask questions or give feedback.

Copy link
Member

@NiklasRentzCAU NiklasRentzCAU left a comment

Choose a reason for hiding this comment

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

Looks good! I left a few minor comments in the code.

@NiklasRentzCAU
Copy link
Member

For clarification of this PR: This is not only the mrtree interactive, but also a cleanup and restructuring of the existing interactive code.
Do you think we should spuash these changes when merging or are the changes in the commits of this PR clean enough?

@NiklasRentzCAU NiklasRentzCAU merged commit 0ee929a into master Mar 14, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants