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

types: Add Expression Typings to Pipeline Stages #11370

Merged
merged 32 commits into from
Jun 17, 2022

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Feb 9, 2022

To be implemented:

  • Arithmetic Expression Operators
  • Array Expression Operators
  • Boolean Expression Operators
  • Comparison Expression Operators
  • Conditional Expression Operators
  • Custom Aggregation Expression Operators
  • Data Size Operators
  • Date Expression Operators
  • Literal Expression Operator
  • Miscellaneous Operators
  • Object Expression Operators
  • Set Expression Operators
  • String Expression Operators
  • Text Expression Operator
  • Trigonometry Expression Operators
  • Type Expression Operators
  • Accumulators
  • Variable Expression Operators
  • Window Operators

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 9, 2022

I guess if I do every day about 20-30 Expressions than in 4-5 days this big chunk of code is done.

Then we can use them for the PipelineStages and Queries. Should be better than the any typings we have currently.

Also there is I think a typescript feature were you can build attribute names based on strings to do stuff like key.$ where key is a key in an interface. But I think this could result in annoying issues like somebody using older typescript could not use the typings or you have keys after a projection stage were you don't have the typings of.
So probably better to just use string as type and don't make it too intelligent.

Anyway, I guess, the best benefit will be when the pipeline stages have better expression support, were you know which fields are existing in an expression.

So this will be the goal of this PR.

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 10, 2022

I should Copy more Of the documentation into the jsdoc.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Couple of minor comments, but LGTM in general. Worst comes to worst re: casting, we just add some any, but I think this change is great for autocomplete

types/OlsonTimezoneCode.d.ts Outdated Show resolved Hide resolved
types/Expression.d.ts Outdated Show resolved Hide resolved
@Uzlopak Uzlopak marked this pull request as draft March 8, 2022 13:37
@Uzlopak Uzlopak marked this pull request as ready for review May 31, 2022 07:48
@Uzlopak Uzlopak requested a review from AbdelrahmanHafez May 31, 2022 07:49
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented May 31, 2022

It should cover all expressions. It is purely based on the mongodb-Documentation.
I think we can risk to merge it. I assume, that we dont cover some edge cases. But it should be easily patched as we have tests and the typings are actually very clean implemented.

@mohammad0-0ahmad

@Uzlopak Uzlopak changed the title WIP: Expression Typings types: Add Expression Typings to Pipeline Stages May 31, 2022
@Uzlopak Uzlopak linked an issue Jun 2, 2022 that may be closed by this pull request
2 tasks
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Jun 7, 2022

@vkarpov15
Can this be part of 6.4?

@vkarpov15 vkarpov15 added this to the 6.4 milestone Jun 15, 2022
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Got a couple of minor comments but LGTM overall

"test/files/*"
"website.js",
"test/files/*",
"benchmarks"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to avoid linting benchmarks and website.js? Doesn't seem like the right thing to do, but I might be missing something

"@typescript-eslint/space-infix-ops": "off"
"overrides": [
{
"files": [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these changes necessary?

/**
* Returns the tangent of a value that is measured in radians.
*
* @version 4.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get rid of the @version tags? It's a bit confusing because @version refers to the MongoDB server rather than the Mongoose version

@vkarpov15 vkarpov15 changed the base branch from master to 6.4 June 17, 2022 02:58
@vkarpov15 vkarpov15 merged commit 47c5f78 into Automattic:6.4 Jun 17, 2022
@andreialecu
Copy link
Contributor

Just a heads up that this PR broke a lot of our aggregations' typings, see more details in #11952

@andreialecu
Copy link
Contributor

I opened #11956 to fix this.

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.

[Types] sort by $meta textScore is not allowed
3 participants