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

#179 Consistently use URI object in APIs #59

Merged
merged 3 commits into from
Nov 15, 2022
Merged

#179 Consistently use URI object in APIs #59

merged 3 commits into from
Nov 15, 2022

Conversation

ndoschek
Copy link
Contributor

Consistently use URI object in APIs

  • Update to latest modelserver-client version
  • Use urijs as typed URI library
    • Use URI objects in APIs and internally for proper URI handling
    • Adapt interfaces, tests, custom examples

General

  • Remove simple-import-sort eslint plugin as it interferes with the built-in import sort
  • Add script to upgrade emfcloud dependencies

Part of eclipse-emfcloud/emfcloud#179

Contributed on behalf of STMicroelectronics


Testing:

  • Use the existing unit and integration tests
  • Use the custom-*-example implementations to test custom commands, routing etc.

- Remove simple-import-sort eslint plugin as it interferes with the built-in import sort
- Add script to upgrade emfcloud dependencies

Contributed on behalf of STMicroelectronics
- Update to latest modelserver-client version
- Use urijs as typed URI library
  - Use URI objects in APIs and internally for proper URI handling
  - Adapt interfaces, tests, custom examples

Part of eclipse-emfcloud/emfcloud#179

Contributed on behalf of STMicroelectronics
@ndoschek ndoschek requested a review from cdamus November 11, 2022 14:06
Copy link
Contributor

@cdamus cdamus left a comment

Choose a reason for hiding this comment

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

Looks great! I have a few comments in-line, but more significantly there are changes needed in the server-integration-test.spec.ts file. I'm perplexed as to why the tests seem to run correctly (when giving them the Java server instance that they need for complete execution).

At lines 47 and 169 of server-integration-test.spec.ts I get compilation errors on a string argument that is now expected to be an URI object. I don't understand why the tests work. I see that in most places in this file the string arguments were previously replaced with URIs, so I think we should finish the job here.

@ndoschek
Copy link
Contributor Author

Thanks @cdamus for your review, I added another commit that addresses your comments.
I am also confused why the compilation didn't show for me on the first cycle, but now I hopefully catched all occurrences of outdated string-uris.

Copy link
Contributor

@cdamus cdamus left a comment

Choose a reason for hiding this comment

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

The new makeUrl(...) change breaks transaction endpoint URLs as shown by test failures. Unfortunately, the build doesn't catch that because all of the transaction-related tests require the Java server and so are skipped in the build, sorry.

- Fix server-integration-tests
- Stabilize transaction url creation
- Use command specific modelUri instead of passed modelUri
Copy link
Contributor

@cdamus cdamus 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 e9ebcb4 into main Nov 15, 2022
@ndoschek ndoschek deleted the issues/179-4 branch November 15, 2022 13:17
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.

3 participants