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.

Sign up for GitHub

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

Jump to bottom

Minor cleanup #88

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Minor cleanup #88

merged 1 commit into from
Jun 21, 2024

Conversation

tortmayr
Copy link
Contributor

  • Change visibility of private fields to protected for better extensibility
  • Update to latest protocol version
    • This introduces a minor unavoidable API break because GIssue has changed from a class to an interface definition
  • Ensure that actions queued with dispatchAfterNextUpdate are also dispatched after the initial SetModelAction
  • Fix copyright header violations

What it does

How to test

Follow-ups

Changelog

  • This PR should be mentioned in the changelog
  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)

@tortmayr
Minor cleanup
aca679e 
- Change visibility of private fields to protected for better extensibility
- Update to latest protocol version
  - This introduces a minor unavoidable API break because GIssue has changed from a class to an interface definition
- Ensure that actions queued with `dispatchAfterNextUpdate` are also dispatched after the initial `SetModelAction`
- Fix copyright header violations
@tortmayr tortmayr requested a review from martin-fleck-at June 20, 2024 20:24
martin-fleck-at
martin-fleck-at reviewed Jun 21, 2024
View reviewed changes
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Change looks good to me in general but I wonder whether we should have simply deprecated EdgeSide, GIssueSeverity, and GIssue to avoid having a breaking change. What do you think?

@tortmayr
Copy link
Contributor Author

Unfortunately that's not possible because the names clash with the definitions provided by @eclipse-glsp/protocol. We cannot export both.
However, in practice the break is really minor, because it only affects direct creation of GIssues on server-side. This is a corner case because typically the server does not directly create issues, instead it creates markers during validation that are applied on the client side and converted to issues.

martin-fleck-at
martin-fleck-at approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thanks for the quick response, then let's merge it as is.

@tortmayr tortmayr merged commit a733f72 into main Jun 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martin-fleck-at martin-fleck-at martin-fleck-at approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tortmayr @martin-fleck-at