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

feat(rosetta): reuse output file as additional cache and introduce --infuse option for extract #3210

Merged
merged 7 commits into from
Nov 30, 2021

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Nov 26, 2021

Introduces two new features for rosetta:extract:

  • rosetta:extract --infuse: combines a common workflow of calling infuse right after extract.
  • extract now uses the output file as an additional cache. This allows us to reuse existing snippets that have already been translated.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 26, 2021
@kaizencc kaizencc changed the title feat(rosetta): --append and --infuse options for extract feat(rosetta): --infuse option for extract and append to output file Nov 30, 2021
Copy link
Contributor Author

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

Some comments on the new lines i added

packages/jsii-rosetta/lib/commands/extract.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Conditional approve, some small things around describing the changes that could be improved.

As does the title: are we still appending? Or do we just overwrite the output tablet with everything from the assembly, but do we reuse existing snippets from the output assembly? That would be a better PR description.

await translator.addToCache(options.cacheTabletFile);
}
await translator.addToCache(options.outputFile);
if (translator.hasCache()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this we can also always try it, and only print if the number of successful reads is > 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

readFromCache will loop through all the snippets and check to see if it exists in the cache. This seems like redundant work if there are no elements in the cache. That is why I added hasCache, not because of the printing.

packages/jsii-rosetta/test/commands/extract.test.ts Outdated Show resolved Hide resolved
@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Nov 30, 2021
@kaizencc kaizencc changed the title feat(rosetta): --infuse option for extract and append to output file feat(rosetta): reuse outputfile as additional cache and introduce --infuse option for extract Nov 30, 2021
@kaizencc kaizencc changed the title feat(rosetta): reuse outputfile as additional cache and introduce --infuse option for extract feat(rosetta): reuse output file as additional cache and introduce --infuse option for extract Nov 30, 2021
@kaizencc kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Nov 30, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 30, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Nov 30, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 30, 2021

Merging (with squash)...

@mergify mergify bot merged commit ccb3c57 into main Nov 30, 2021
@mergify mergify bot deleted the extract-append branch November 30, 2021 22:42
@mergify
Copy link
Contributor

mergify bot commented Nov 30, 2021

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants