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

Reduce usage of "any" #448

Merged
merged 6 commits into from
May 22, 2024
Merged

Conversation

demetris-manikas
Copy link
Contributor

This PR aims to reduce the usage of any in the codebase

@demetris-manikas demetris-manikas force-pushed the reduce-usage-of-any branch 2 times, most recently from 7cbfd1a to 383a962 Compare May 18, 2024 20:45
Copy link
Collaborator

@devhawk devhawk left a comment

Choose a reason for hiding this comment

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

LGTM. Nice change

@devhawk
Copy link
Collaborator

devhawk commented May 21, 2024

@demetris-manikas I'm thinking we should also look at simplifying UserDatabase + UserDatabaseQuery + UserDatabaseTransaction types to define params/args parameters as unknown[] type instead of having a type argument T extends unknown[]. Thoughts?

@demetris-manikas
Copy link
Contributor Author

demetris-manikas commented May 22, 2024

Definitely. <T extends unknown[]> is kind of non-existent as a concept :)

Still it proved a non trivial change.
Better do it in a separate PR

@demetris-manikas demetris-manikas marked this pull request as draft May 22, 2024 16:53
@devhawk devhawk marked this pull request as ready for review May 22, 2024 16:54
@devhawk
Copy link
Collaborator

devhawk commented May 22, 2024

@demetris-manikas I thought I had accidently marked this as draft and switched it back. Did you mean to switch this?

@demetris-manikas
Copy link
Contributor Author

I converted this to daft cause after rebasing to the latest code it gives me some linting errors.

@devhawk
Copy link
Collaborator

devhawk commented May 22, 2024

OK I was going to merge but I'll wait for you to address the linter issues. Sorry for the confusion

@devhawk
Copy link
Collaborator

devhawk commented May 22, 2024

@demetris-manikas please let me know when you're ready for me to merge this

@demetris-manikas
Copy link
Contributor Author

No problem. These things happen all the time

@demetris-manikas
Copy link
Contributor Author

demetris-manikas commented May 22, 2024

I updated the packages and the linting errors vanished.
I get a warning though that the typescript version 5.4.5 is not officially supported from the linter but I guess you have all seen that.
You can go ahead and merge

@devhawk
Copy link
Collaborator

devhawk commented May 22, 2024

I opened #458 to track updating the linter

@devhawk devhawk merged commit 8f1b5d4 into dbos-inc:main May 22, 2024
2 checks passed
@kraftp kraftp mentioned this pull request May 23, 2024
kraftp added a commit that referenced this pull request May 23, 2024
kraftp added a commit that referenced this pull request May 23, 2024
This reverts commit 8f1b5d4 as it
included breaking API changes. It caused numerous failures on our demo
applications and in our staging environment and cannot be released in
its current state. The changes in that PR are good, but we need to
introduce them gradually and carefully (likely in multiple smaller PRs)
and verify they don't cause regressions.
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.

2 participants