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

Refactoring of target properties and their validation #2008

Merged
merged 151 commits into from
Nov 3, 2023
Merged

Conversation

lhstrh
Copy link
Member

@lhstrh lhstrh commented Sep 15, 2023

This PR reworks the way we deal with target properties. It makes them easier to specify, manipulate, and verify. It cuts down on the use of lambda functions, which makes the code a bit more verbose, but also more reusable and more readable. The TargetProperty class that had become a depository of random helper classes has been cleaned and brought down from 1800 LOC to less than 300.

While the goal of this refactoring is to retain the same functionality, some changes were made in the name of enforcing consistency:

  • --target-compiler CLI argument changed to --compiler so that it matches the compiler target property
  • error reporting changed to report more precisely (on the Element instead of KeyValuePair) and with simpler messages
  • Unrecognized platform causes NPE #1919
  • Stateless target properties #2039
  • let TypeScript backend honor --no-compile flag/target property
  • let Rust and C++ use the same build type (Debug)
  • register target properties in Target class and revise "supported by" mechanism
  • add method for parsing from JSON (for interop with lingo)
  • base CLI args directly on target property names
  • clear separation between configurators and transformers in the test framework (c96a794)
  • add comments

Fixes #1558.
Fixes #2001.
Fixes #1919.
Fixes #1859.

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 is a great improvement! I haven't looked carefully at all the changes yet, but I left some comments in places where I noticed issues or had questions.

core/src/main/java/org/lflang/TargetConfig.java Outdated Show resolved Hide resolved
core/src/main/java/org/lflang/TargetProperty.java Outdated Show resolved Hide resolved
core/src/main/java/org/lflang/TargetProperty.java Outdated Show resolved Hide resolved
@lhstrh lhstrh requested a review from cmnrd November 3, 2023 03:39
@lhstrh lhstrh enabled auto-merge November 3, 2023 03:39
Copy link
Collaborator

@lsk567 lsk567 left a comment

Choose a reason for hiding this comment

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

This is a big effort that makes target properties more robust by relying less on String operations. Looks good to me overall. Thanks for working on this @lhstrh. :-)

@lhstrh lhstrh added this pull request to the merge queue Nov 3, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 3, 2023
@lhstrh lhstrh added this pull request to the merge queue Nov 3, 2023
Merged via the queue into master with commit f92b6d9 Nov 3, 2023
41 checks passed
@lhstrh lhstrh deleted the target-properties branch November 3, 2023 08:39
@lhstrh lhstrh added the refactoring Code quality enhancement label Jan 23, 2024
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
5 participants