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

Add SCXML language module #1030

Merged
merged 10 commits into from
Apr 26, 2023
Merged

Add SCXML language module #1030

merged 10 commits into from
Apr 26, 2023

Conversation

smjonas
Copy link
Contributor

@smjonas smjonas commented Apr 15, 2023

Adds support for statecharts defined in the SCXML format.

@tsaglam tsaglam added enhancement Issue/PR that involves features, improvements and other changes major Major issue/feature/contribution/change language PR / Issue deals (partly) with new and/or existing languages for JPlag labels Apr 17, 2023
@tsaglam tsaglam self-requested a review April 17, 2023 06:30
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

@smjonas, there are a few issues you need to resolve.

languages/scxml/README.md Outdated Show resolved Hide resolved
languages/scxml/README.md Outdated Show resolved Hide resolved
* its type is stored. This can be used to "peek" at a list of token types that are extracted when visiting a
* statechart.
*/
public class PeekAdapter extends ScxmlParserAdapter {
Copy link
Member

Choose a reason for hiding this comment

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

You extend the parser adapter. Thus this class is still functionally a full parser adapter. The name and comment, however, do not reflect that. Could you elaborate on the design here?

@tsaglam
Copy link
Member

tsaglam commented Apr 21, 2023

@smjonas, please also fix the build error and apply spotless. If there are any issues, let me know.
Since now I left a review, avoid force pushes.

Copy link
Member

@dfuchss dfuchss left a comment

Choose a reason for hiding this comment

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

Additionally the tests build (javadoc) needs to be fixed :)

@tsaglam
Copy link
Member

tsaglam commented Apr 25, 2023

@smjonas after you implemented our feedback, take a look at the code smells detected by Sonar and fix them! 👍

@smjonas smjonas force-pushed the scxml branch 4 times, most recently from c81dc82 to 6304ce9 Compare April 25, 2023 09:48
@smjonas
Copy link
Contributor Author

smjonas commented Apr 25, 2023

@tsaglam The remaining detected code smell seems to be a false-positive as the explicit constructor performs additional checks

@dfuchss
Copy link
Member

dfuchss commented Apr 25, 2023

@tsaglam The remaining detected code smell seems to be a false-positive as the explicit constructor performs additional checks

Maybe sonarlint is just complaining about the redundancy of the parameters. In records you can omit the parameters (and brackets) of the default constructor. E.g., https://www.baeldung.com/java-records-custom-constructor (3.)

@smjonas
Copy link
Contributor Author

smjonas commented Apr 25, 2023

I tried to change it to

    /**
     * Constructs a new state.
     * @throws IllegalArgumentException if {@code transitions} or {@code substates} is null
     */
    public State {
        if (transitions == null) {
            throw new IllegalArgumentException("State.transitions must not be null");
        }
        if (substates == null) {
            throw new IllegalArgumentException("State.substates must not be null");
        }
        updateTimedTransitions();
    }

but that won't work because updateTimedTransitions requires all variables to be initialized. So unless anyone has a better idea how to ensure that updateTimedTransition is called during instance construction, I'll have to leave the code in its current state (which is

    /**
     * Constructs a new state.
     * @throws IllegalArgumentException if {@code transitions} or {@code substates} is null
     */
    public State(String id, List<Transition> transitions, List<State> substates, List<Action> actions, boolean initial, boolean parallel) {
        if (transitions == null) {
            throw new IllegalArgumentException("State.transitions must not be null");
        }

        if (substates == null) {
            throw new IllegalArgumentException("State.substates must not be null");
        }

        this.id = id;
        this.transitions = transitions;
        this.substates = substates;
        this.actions = actions;
        this.initial = initial;
        this.parallel = parallel;
        updateTimedTransitions();
    }

).

@sonarcloud
Copy link

sonarcloud bot commented Apr 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

91.0% 91.0% Coverage
1.5% 1.5% Duplication

@dfuchss
Copy link
Member

dfuchss commented Apr 25, 2023

Seems to be false positive. I'll mark it that way.

@tsaglam tsaglam merged commit 3c05d4a into jplag:develop Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR that involves features, improvements and other changes language PR / Issue deals (partly) with new and/or existing languages for JPlag major Major issue/feature/contribution/change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants