Skip to content

Missing typings and some fixes #487

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
Merged
merged 3 commits into from
Aug 24, 2017
Merged

Missing typings and some fixes #487

merged 3 commits into from
Aug 24, 2017

Conversation

@elhigu
Copy link
Contributor

@elhigu elhigu commented Aug 23, 2017

No description provided.

@coveralls
Copy link

coveralls commented Aug 23, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 94.392% when pulling a8fbd5d on elhigu:missing-typings-and-some-fixes into d2e4062 on Vincit:master.
koskimas
koskimas reviewed Aug 23, 2017
View reviewed changes
typings/objection/index.d.ts Outdated
// Non-query methods:

context(queryContext: object): this;
context(): object;
Copy link
Collaborator

@koskimas koskimas Aug 23, 2017

Choose a reason for hiding this comment

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

Isn't object as a return value pretty useless? You cannot access any property through it without first casting it to any or some other type. This should return QueryContext type.
Copy link
Contributor Author

@elhigu elhigu Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes they are pretty useless, I just took the same type out that was passed in in existing context(queryContext: object): this; declaration. Is there any problems if I change both of them to QueryContext type?
Copy link
Collaborator

@koskimas koskimas Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

object as an argument is ok since any object can be downcasted to object. The argument shouldn't be QueryContext because QueryContext type contains some properties that are added by objection and should never be passed to context or mergeContext methods.
Copy link
Contributor Author

@elhigu elhigu Aug 24, 2017

Choose a reason for hiding this comment

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

Ah right, now I get it. I'll just fix the new API declaration.
@elhigu elhigu force-pushed the missing-typings-and-some-fixes branch from a8fbd5d to 3a97f94 Compare August 24, 2017 11:33
@coveralls
Copy link

coveralls commented Aug 24, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 94.392% when pulling 3a97f94 on elhigu:missing-typings-and-some-fixes into d2e4062 on Vincit:master.
@koskimas koskimas merged commit e13413d into Vincit:master Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@elhigu @coveralls @koskimas