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

enhancement(lua transform): Add version configuration option #2056

Merged
9 commits merged into from
Mar 16, 2020

Conversation

ghost
Copy link

@ghost ghost commented Mar 13, 2020

This PR adds version configuration option to the lua transform. The version can take values 1 and 2, the default values is 1.

This also splits implementation for both versions, so all further changes on version 2 would not affect version 1.

Ref #2000.

@ghost ghost requested a review from binarylogic as a code owner March 13, 2020 15:46
@github-actions
Copy link

github-actions bot commented Mar 13, 2020

Great PR! Please pay attention to the following items before merging:

Files matching src/**:

  • For each failure path, is there sufficient context logged for users to investigate the issue?
  • Do the tests ensure that behavior is sane for inputs that don't meet normal assumptions (e.g. missing field, non-string, etc)?
  • Did you add adequate documentation?

Files matching src/transforms/**:

  • Did you add behavior tests (tests/behavior) that represent the behavior of this change? For example, testing for Vector's field path notation for nested fields.

This is an automatically generated QA checklist based on modified files

required = false
description = "transform API version"
default = "1"
enum = { 1 = "transform API version 1", 2 = "transform API version 2" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove 2 until we actually support it. And, at the time, we should make sure it what is presented in the configuration example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, you do have a v2 here. Does that implement the API discussed in the spec?

Copy link
Author

@ghost ghost Mar 13, 2020

Choose a reason for hiding this comment

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

Ah, I see, you do have a v2 here. Does that implement the API discussed in the spec?

It is currently a duplicate of v1. I'm planning to gradually implement features from the RFC in v2, so although it is available, the exposed API for it will be very unstable until most of the features are there. So it makes sense to hide it until it is somewhat stabilized.

Copy link
Contributor

@binarylogic binarylogic left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@ghost ghost merged commit 874c0c0 into master Mar 16, 2020
@ghost ghost deleted the lua-transform-version-option branch March 25, 2020 17:12
This pull request was closed.
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