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

#712 Align GGraph model with newest changes from glsp-server #22

Merged
merged 2 commits into from
Jul 15, 2022

Conversation

ndoschek
Copy link
Contributor

@ndoschek ndoschek commented Jul 14, 2022

  • Align model in packages/graph with GGraph/SGraph
    • Add missing interface GHtmlRoot
    • Add missing static builder function for GIssueMarker
    • Rename files to match class/interface name
  • Update build scripts and VSCode build task
  • Update CHANGELOG

Resolves eclipse-glsp/glsp#712

Contributed on behalf of STMicroelectronics.

@ndoschek ndoschek force-pushed the issues/712 branch 2 times, most recently from bc11317 to 86b5083 Compare July 15, 2022 08:04
@ndoschek ndoschek requested a review from tortmayr July 15, 2022 08:04
packages/graph/src/gissue.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/graph/src/default-types.ts Outdated Show resolved Hide resolved
packages/graph/src/default-types.ts Outdated Show resolved Hide resolved
packages/graph/src/default-types.ts Outdated Show resolved Hide resolved
packages/graph/src/default-types.ts Outdated Show resolved Hide resolved
packages/graph/src/default-types.ts Outdated Show resolved Hide resolved
packages/graph/src/gargumentable.ts Outdated Show resolved Hide resolved
packages/graph/src/gedge-layoutable.ts Show resolved Hide resolved
packages/graph/src/gshape-prerendered-element.ts Outdated Show resolved Hide resolved
- Align model in packages/graph with GGraph/SGraph
  - Add missing interface GHtmlRoot
  - Add missing static builder function for GIssueMarker
  - Rename files to match class/interface name
- Update build scripts and VSCode build task
- Update CHANGELOG

Resolves eclipse-glsp/glsp#712

Contributed on behalf of STMicroelectronics.
@ndoschek
Copy link
Contributor Author

Thanks @tortmayr for your review, I updated my commit, would be great if you could have another look thanks!

@ndoschek ndoschek requested a review from tortmayr July 15, 2022 09:56
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks Nina. Code looks good to me and everything still works as expected.
I only have one small remark regarding the changelog

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated
- `GLayoutContainer` (`glayout-container.ts`) -> `GLayouting` (`glayouting.ts`) (affected classes: `GCompartment`, `GGraph`, `GNode`)
- `GShapePreRenderedElement` (`gpre-shape-prerendered-element.ts`) -> `GShapedPreRenderedElement` (`gshaped-prerendered-element.ts`)
- Renamed file names: `gbound-aware.ts` -> `gbounds-aware.ts`
- `GGraph` now additionally extends `GLayoutable`
Copy link
Contributor

Choose a reason for hiding this comment

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

API additions don' need to be mentioned in the breaking change section (as they don't break any existing API 😉)
Please remove the last three lines

CHANGELOG.md Outdated
- [graph] Align GGraph model with newest changes from glsp-server [#22](https://github.com/eclipse-glsp/glsp-server-node/pull/22) - Contributed on behalf of STMicroelectronics
- Renamed interfaces:
- `EdgePlacement` -> `GEdgePlacement` (affected classes: `GEdgeLayoutable`, `GLabel`)
- `GLayoutContainer` (`glayout-container.ts`) -> `GLayouting` (`glayouting.ts`) (affected classes: `GCompartment`, `GGraph`, `GNode`)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can omit the file information here ('glayout-container.ts`) etc. GLayoutContainer is default exported so a source file name changes does not break anything for adopters.

@tortmayr tortmayr self-requested a review July 15, 2022 13:02
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks 👍

@ndoschek ndoschek merged commit afa6928 into main Jul 15, 2022
@ndoschek ndoschek deleted the issues/712 branch July 15, 2022 13:05
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.

Align GGraph model with newest changes from glsp-server
2 participants