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

Ported LFValidator.xtend to Java #886

Merged
merged 26 commits into from
Feb 4, 2022
Merged

Ported LFValidator.xtend to Java #886

merged 26 commits into from
Feb 4, 2022

Conversation

housengw
Copy link
Contributor

@housengw housengw commented Jan 25, 2022

see #838

@lhstrh
Copy link
Member

lhstrh commented Jan 26, 2022

The validator is a very important class, and it will be difficult to assure the ported code is functionally correct. I would suggest that before merging this in, we improve code coverage of LFValidator with unit tests.

@lhstrh
Copy link
Member

lhstrh commented Jan 26, 2022

@housengw could you incorporate the following changes in your port please?
https://github.com/lf-lang/lingua-franca/pull/804/files
In your commit message, please reference PR #804 so that is shows up in the conversation of that PR.
Once this has been done, I will close #804.

@lhstrh
Copy link
Member

lhstrh commented Jan 26, 2022

The dummy Xtend file shouldn't be needed if you set generateXtendStub = true to generateXtendStub = false in org.lflang/src/org/lflang/GenerateLinguaFranca.mwe2

@housengw housengw marked this pull request as ready for review January 27, 2022 21:54
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

This looks great! Due to the size of the Validator, it's not possible to check every line, but I made a quick read through. One issue that I noticed, is that the checkWidthSpec method disappeared in the Java version.

Also, when opening the java file in IDEA, there is a ton of warnings reported. Many of which suggest code improvements. This is not to say that your code is bad, but more to say that we need to think about our developer setups and what are the recommended tools for working with our code base should be. Luckily, when we don't rely on xtend anymore, we have more options then eclipse :)

@Soroosh129
Copy link
Contributor

Soroosh129 commented Jan 28, 2022

Looks great!

I wonder if we should delay the task of porting Xtend files that can heavily benefit from Java 17's multi-line strings until after we make the jump (which might happen soon). It seems like leaving separate (untested) Java 17 code blocks as comment will just lead to a lot of duplicate work down the line.

Edit: As @lhstrh mentioned here, we might not get to use Java 17 anytime soon. Looking at Xtext's past release cycles, the next next release could be at least 4 months away. So I see the reasoning behind doing this now.

@cmnrd
Copy link
Collaborator

cmnrd commented Jan 28, 2022

I think eventually we will anyway need to do another pass over the core classes. There are lots of opportunities for designing the validator in a cleaner and more modular way. In such a go we could also modernize the Java code or, if we don't need to rely on Eclipse for development anymore (which should be the case once we have thrown out all xtend), we could also port the class entirely to Kotlin. With IntelliJ's tools for auto-conversion and code linting, this should be a walk in the park compared to the transition from xtend to java ;)

@lhstrh
Copy link
Member

lhstrh commented Jan 29, 2022

Adding a link to my comment in #873.

@lhstrh
Copy link
Member

lhstrh commented Feb 1, 2022

I think we should merge this after tagging 0.1.0-beta.

@housengw housengw merged commit 9a0f1e1 into master Feb 4, 2022
@oowekyala oowekyala deleted the xtend-to-java-validation branch February 5, 2022 13:57
@lhstrh lhstrh changed the title port LFValidator.xtend to java Final port of LFValidator.xtend to Java Mar 17, 2022
@lhstrh lhstrh changed the title Final port of LFValidator.xtend to Java Port of LFValidator.xtend to Java Mar 17, 2022
@lhstrh lhstrh changed the title Port of LFValidator.xtend to Java Ported LFValidator.xtend to Java Mar 17, 2022
@lhstrh lhstrh added the refactoring Code quality enhancement label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants