Skip to content

feat: allow ast transformers to take in addl opts, fix #1942#1946

Closed
longlho wants to merge 3 commits intokulshekhar:masterfrom
longlho:opts
Closed

feat: allow ast transformers to take in addl opts, fix #1942#1946
longlho wants to merge 3 commits intokulshekhar:masterfrom
longlho:opts

Conversation

@longlho
Copy link
Copy Markdown
Contributor

@longlho longlho commented Sep 12, 2020

Summary

See #1942

Test plan

Changed existing unit test

Does this PR introduce a breaking change?

  • No

Other information

@longlho longlho requested a review from kulshekhar as a code owner September 12, 2020 16:53
@longlho
Copy link
Copy Markdown
Contributor Author

longlho commented Sep 12, 2020

cc @ahnpnl

@ahnpnl
Copy link
Copy Markdown
Collaborator

ahnpnl commented Sep 12, 2020

nice ! So actually just simply pass the option to factory.

Copy link
Copy Markdown
Collaborator

@ahnpnl ahnpnl left a comment

Choose a reason for hiding this comment

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

the codes LGTM, only missing unit tests and e2e tests

Comment thread e2e/__tests__/path-mapping.test.ts Outdated
Comment thread docs/user/config/astTransformers.md
Comment thread src/config/config-set.ts Outdated
@longlho
Copy link
Copy Markdown
Contributor Author

longlho commented Sep 13, 2020

@ahnpnl I'm not sure how to add e2e test since the doc is broken. Also idk why existing test failed :-/ Can u provide some help?

@ahnpnl
Copy link
Copy Markdown
Collaborator

ahnpnl commented Sep 13, 2020

You can follow these steps:

@ahnpnl
Copy link
Copy Markdown
Collaborator

ahnpnl commented Sep 17, 2020

Do you get somewhere with e2e tests ? I can also take over if you don’t mind :)

@longlho
Copy link
Copy Markdown
Contributor Author

longlho commented Sep 17, 2020 via email

@ahnpnl
Copy link
Copy Markdown
Collaborator

ahnpnl commented Sep 18, 2020

Continue in #1966

@ahnpnl ahnpnl closed this Sep 18, 2020
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