Skip to content

Conversation

addaleax
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Looks good!

});
it('creates a view that potentially contains JS functions in its pipeline', async() => {
const pipeline = (body: any) => [{
'$set': {
Copy link
Collaborator

@gribnoysup gribnoysup Feb 12, 2021

Choose a reason for hiding this comment

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

I see that tests are failing, maybe this should be addFields instead? (Big disclaimer: I'm not really good with aggregations so not sure if what I'm suggesting matches what you wanted to achieve with the pipeline 😅)

Suggested change
'$set': {
'$addFields': {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I’m also fairly clueless here. This passed locally and was taken directly from the ticket, and it looks like it’s the cli-service-provider tests that are failing (because I didn’t run them locally, which is entirely my fault, but … 🤷)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did some googling around and seems like $set is a new feature in 4.2 and $function is new in 4.4. Should we skip that test for anything that is less than that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just pushed a commit for that, yes :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Haha, ~ at the same time, perfect timing 😄

@addaleax addaleax merged commit 8b5a2b1 into master Feb 15, 2021
@addaleax addaleax deleted the 585-dev branch February 15, 2021 14:20
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