Skip to content

Conversation

prestonvasquez
Copy link
Contributor

@prestonvasquez prestonvasquez commented Apr 7, 2022

Description

COMPASS-5685

See the the updated README.md for the bson-transpilers package.

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

The go driver export has a need to prepend the driver syntax with variable declarations to avoid using closures within the CRUD operations. For example, the following would be what is generated under the current architecture:

// This would be embedded in some CRUD Operation and is what would be transpiled from
// { _id: ObjectID("5ab901c29ee65f5c8550c5b9") }
bson.D{{"_id", func(hex string) primitive.ObjectID {
  objectID, err := primitive.ObjectIDFromHex(hex)
  if err != nil {
    log.Fatal(err)
  }
  return objectID
}("5ab901c29ee65f5c8550c5b9")}}

The proposed changes would add an optional functionality to prepend the document body with variable declarations

objectID err := primitive.ObjectIDFromHex("5ab901c29ee65f5c8550c5b9")
if err != nil {
  log.Fatal(err)
}
 
// this would be embedded in some CRUD Operation
bson.D{{"_id", objectID}}
  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

  1. Does the method for passing the declarationStore into the argsTemplate hold up? That is, is it best practices?
  2. Does DeclarationStore follow good JS class-naming standards?

Dependents

GODRIVER-2268

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@prestonvasquez prestonvasquez marked this pull request as draft April 7, 2022 20:11
@prestonvasquez prestonvasquez marked this pull request as ready for review April 7, 2022 20:12
@prestonvasquez prestonvasquez marked this pull request as draft April 7, 2022 20:12
@prestonvasquez prestonvasquez marked this pull request as ready for review April 7, 2022 20:13
@alenakhineika
Copy link
Contributor

Looks good! Since you are updating readme, could you please also add ruby to the output languages at the beginning of the readme file? We forgot to do it when added its support.

prestonvasquez and others added 5 commits April 8, 2022 09:03
Co-authored-by: Anna Henningsen <github@addaleax.net>
Co-authored-by: Anna Henningsen <github@addaleax.net>
…f github.com:prestonvasquez/compass into COMPASS-5685.addVariableDeclarationsToBSONTranspiler
@prestonvasquez prestonvasquez requested a review from addaleax April 8, 2022 15:08
super();
this.idiomatic = true; // PUBLIC
this.clearImports();
this.state = { declarationStore: new DeclarationStore() };
Copy link
Contributor Author

@prestonvasquez prestonvasquez Apr 11, 2022

Choose a reason for hiding this comment

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

@alenakhineika @addaleax @mcasimir I ran into an issue concerning the way we handle the argsTemplate functions. Since they are variadic, passing in the declaration store as the last value will cause a lot of problems with existing language templates. This one, in particular, comes to mind. To fix this, I propose

  1. Adding this state hash in case we ever need to add state for something else in the future
  2. Updating all uses of argsTemplate so that the first argument will be be this.state

Here is a more concise explanation.

I played around a bit with adding a new template type altogether, but to add this functionality safely we'd have to update all the templates to include that new type. Given that, using the existing args templates seemed like a more robust solution.

start(ctx) {
return this.returnResult(ctx).trim();
start(ctx, useDeclarations = false) {
return (useDeclarations ? this.returnResultWithDeclarations(ctx) : this.returnResult(ctx)).trim();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is definitely of note, it's what allows us to prepend the raw document output with functions/variables:

Screen Shot 2022-04-13 at 11 41 22 AM

There might be a much better way of going about this, though.

@prestonvasquez prestonvasquez changed the title Add DeclarationStore to transpiler (POC) feat(export-to-language): Add DeclarationStore to transpiler Apr 13, 2022
@prestonvasquez
Copy link
Contributor Author

@matthewdale @benjirewis Here is the final draft for the code to prepend declarations to statements.


Transpilers for building BSON documents in any language. Current support
provided for `shell` `javascript` and `python` as inputs. `java`, `c#`, `node`, `shell` and `python` as
provided for `shell` `javascript` and `python` as inputs. `java`, `c#`, `node`, `shell`, `python` and `ruby` as
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say go?

Copy link
Contributor Author

@prestonvasquez prestonvasquez Apr 15, 2022

Choose a reason for hiding this comment

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

Ah yeah, we should definitely include go. I'll update that on #2991 so that this PR can remain language agnostic. I added ruby for this request.

}
```
The output of the driver syntax for this language will be the one-line statement `Hello World`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! 🧑‍🔧

args = visitedElements.map((arg, index) => {
const last = !visitedElements[index + 1];
return ctx.type.argsTemplate(arg, ctx.indentDepth, last);
return ctx.type.argsTemplate.bind(this.getState())(arg, ctx.indentDepth, last);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does bind just merge the variables in state with the ones already in this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually replaces the existing this of the prototypes with state. The solutions seems safe based on my conversations with @mcasimir , but there may definitely be underlying problems I'm not aware of. An alternative would be to merge the two objects before the bind.


assert.strictEqual(ds.length(), 3);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Great testing 🧑‍🔧

prestonvasquez and others added 2 commits April 15, 2022 09:14
@mcasimir mcasimir closed this May 12, 2022
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.

5 participants