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

feat(TS): Changed tests to TypeScript and updated typings #136

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

ffMathy
Copy link
Contributor

@ffMathy ffMathy commented Sep 2, 2023

I converted all tests to TypeScript in this PR.

As part of doing that, I noticed some places where __entity_type__ was not included in the types, and some places where an entity_key was passed as a string in the tests, but was expecting a string-array. Please double-check these typing changes.

There were some cases where a private method was called from a test. This should probably be avoided in the future, but for those cases for now, I added @ts-ignore.

There are also some cases where I defined a type as any because the type was too complex for me to model, but this gets it quite far.

@ffMathy ffMathy requested a review from a team as a code owner September 2, 2023 12:15
Copy link
Contributor

@gismya gismya left a comment

Choose a reason for hiding this comment

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

Nicely done. Good find with the entity key for update and delete. Minor issues with the test conversions

test/convert_to_iso_string.test.ts Outdated Show resolved Hide resolved
test/convert_to_iso_string.test.ts Outdated Show resolved Hide resolved
test/convert_to_iso_string.test.ts Outdated Show resolved Hide resolved
test/server.ts Outdated Show resolved Hide resolved
source/operation.ts Show resolved Hide resolved
test/session.test.ts Outdated Show resolved Hide resolved
test/session.test.ts Show resolved Hide resolved
test/session.test.ts Outdated Show resolved Hide resolved
test/session.test.ts Show resolved Hide resolved
test/session.test.ts Outdated Show resolved Hide resolved
@ffMathy
Copy link
Contributor Author

ffMathy commented Sep 7, 2023

Excellent feedback! I made some changes.

@ffMathy
Copy link
Contributor Author

ffMathy commented Sep 11, 2023

I think everything has been resolved now! 🤞

@gismya gismya changed the title Changed tests to TypeScript and updated typings feat(TS): Changed tests to TypeScript and updated typings Sep 11, 2023
@gismya gismya merged commit 8dddf76 into ftrackhq:main Sep 11, 2023
1 of 3 checks passed
gismya pushed a commit that referenced this pull request Sep 11, 2023
* Changed tests to TypeScript and updated typings.

* fix: review changes
gismya pushed a commit that referenced this pull request Sep 11, 2023
* Changed tests to TypeScript and updated typings.

* fix: review changes
gismya added a commit that referenced this pull request Sep 11, 2023
* fix: publishReply has incorrect Event in reply

* Corrected source preparation

* Apply suggestions from code review

Co-authored-by: Lucas Stålner Correia <lucas.correia@ftrack.com>

* PR suggestions

* feat(TS): Changed tests to TypeScript and updated typings (#136)

* Changed tests to TypeScript and updated typings.

* fix: review changes

* feat(TS): Changed tests to TypeScript and updated typings (#136)

* Changed tests to TypeScript and updated typings.

* fix: review changes

* Empty-merge

* Event dropped out in merges

---------

Co-authored-by: Lucas Stålner Correia <lucas.correia@ftrack.com>
Co-authored-by: Mathias Lykkegaard Lorenzen <mathias.lykkegaard.lorenzen@lego.com>
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