From 874e8e2c6a4cc61ae049c5b23f25ff390504d345 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 8 Nov 2021 12:02:52 +0100 Subject: [PATCH] feat(pacmak): fail on untranslated snippets (#3127) Because of backwards compatibility reasons, pacmak will try to live-translate example code it encounters. However, this can add unpredictably to the compile times, especially while we're making sure that rosetta and pacmak agree on API locations for all snippets that are being translated (which are now part of the snippet keys). Add an option to pacmak to fail if any snippets are encountered that are not pre-translated in any of the tablets. This most likely signifies an error. This will be enabled in cdklabs/cdk-ops#1777. --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0 --- packages/jsii-pacmak/bin/jsii-pacmak.ts | 17 ++++- packages/jsii-pacmak/lib/index.ts | 19 +++++- .../lib/commands/transliterate.ts | 4 +- packages/jsii-rosetta/lib/rosetta.ts | 66 +++++++++++++++---- packages/jsii-rosetta/test/rosetta.test.ts | 33 +++++++++- 5 files changed, 120 insertions(+), 19 deletions(-) diff --git a/packages/jsii-pacmak/bin/jsii-pacmak.ts b/packages/jsii-pacmak/bin/jsii-pacmak.ts index dbb02105aa..35f39df32d 100644 --- a/packages/jsii-pacmak/bin/jsii-pacmak.ts +++ b/packages/jsii-pacmak/bin/jsii-pacmak.ts @@ -1,6 +1,7 @@ #!/usr/bin/env node import '@jsii/check-node/run'; +import { UnknownSnippetMode } from 'jsii-rosetta'; import * as yargs from 'yargs'; import { pacmak, configureLogging, TargetName } from '../lib'; @@ -93,9 +94,20 @@ import { VERSION_DESC } from '../lib/version'; }) .option('rosetta-translate-live', { type: 'boolean', - desc: "Translate code samples on-the-fly if they can't be found in the samples tablet", + desc: "Translate code samples on-the-fly if they can't be found in the samples tablet (deprecated)", default: true, }) + .option('rosetta-unknown-snippets', { + type: 'string', + requiresArg: true, + optional: true, + choices: [ + UnknownSnippetMode.VERBATIM, + UnknownSnippetMode.TRANSLATE, + UnknownSnippetMode.FAIL, + ], + desc: "What to do with code samples if they can't be found in the samples tablet", + }) .option('parallel', { type: 'boolean', desc: 'Generate all configured targets in parallel (disabling this might help if you encounter EMFILE errors)', @@ -136,6 +148,9 @@ import { VERSION_DESC } from '../lib/version'; parallel: argv.parallel, recurse: argv.recurse, rosettaLiveConversion: argv['rosetta-translate-live'], + rosettaUnknownSnippets: argv['rosetta-unknown-snippets'] as + | UnknownSnippetMode + | undefined, rosettaTablet: argv['rosetta-tablet'], targets: argv.targets?.map((target) => target as TargetName), updateNpmIgnoreFiles: argv.npmignore, diff --git a/packages/jsii-pacmak/lib/index.ts b/packages/jsii-pacmak/lib/index.ts index c953c53502..29098cf533 100644 --- a/packages/jsii-pacmak/lib/index.ts +++ b/packages/jsii-pacmak/lib/index.ts @@ -1,5 +1,5 @@ import { TypeSystem } from 'jsii-reflect'; -import { Rosetta } from 'jsii-rosetta'; +import { Rosetta, UnknownSnippetMode } from 'jsii-rosetta'; import * as logging from './logging'; import { findJsiiModules, updateAllNpmIgnores } from './npm-modules'; @@ -31,10 +31,17 @@ export async function pacmak({ rosettaTablet, targets = Object.values(TargetName), timers = new Timers(), + rosettaUnknownSnippets = undefined, updateNpmIgnoreFiles = false, validateAssemblies = false, }: PacmakOptions): Promise { - const rosetta = new Rosetta({ liveConversion: rosettaLiveConversion }); + const unknownSnippets = + rosettaUnknownSnippets ?? + (rosettaLiveConversion + ? UnknownSnippetMode.TRANSLATE + : UnknownSnippetMode.VERBATIM); + + const rosetta = new Rosetta({ unknownSnippets }); if (rosettaTablet) { await rosetta.loadTabletFromFile(rosettaTablet); } @@ -219,9 +226,17 @@ export interface PacmakOptions { * already translated in the `rosettaTablet` file. * * @default false + * @deprecated Use `rosettaUnknownSnippets` instead. */ readonly rosettaLiveConversion?: boolean; + /** + * How rosetta should treat snippets that cannot be loaded from a translation tablet. + * + * @default - falls back to the default of `rosettaLiveConversion`. + */ + readonly rosettaUnknownSnippets?: UnknownSnippetMode; + /** * A Rosetta tablet file where translations for code examples can be found. * diff --git a/packages/jsii-rosetta/lib/commands/transliterate.ts b/packages/jsii-rosetta/lib/commands/transliterate.ts index 23432d9c31..082c5a85f5 100644 --- a/packages/jsii-rosetta/lib/commands/transliterate.ts +++ b/packages/jsii-rosetta/lib/commands/transliterate.ts @@ -5,7 +5,7 @@ import { resolve } from 'path'; import { fixturize } from '../fixtures'; import { TargetLanguage } from '../languages'; import { debug } from '../logging'; -import { Rosetta } from '../rosetta'; +import { Rosetta, UnknownSnippetMode } from '../rosetta'; import { SnippetParameters, typeScriptSnippetFromSource, ApiLocation } from '../snippet'; import { Translation } from '../tablets/tablets'; @@ -53,7 +53,7 @@ export async function transliterateAssembly( ): Promise { const rosetta = new Rosetta({ includeCompilerDiagnostics: true, - liveConversion: true, + unknownSnippets: UnknownSnippetMode.TRANSLATE, loose: options.loose, targetLanguages, }); diff --git a/packages/jsii-rosetta/lib/rosetta.ts b/packages/jsii-rosetta/lib/rosetta.ts index babacb94ec..77e60f7c02 100644 --- a/packages/jsii-rosetta/lib/rosetta.ts +++ b/packages/jsii-rosetta/lib/rosetta.ts @@ -17,17 +17,35 @@ import { ApiLocation, typeScriptSnippetFromSource, } from './snippet'; +import { snippetKey } from './tablets/key'; import { DEFAULT_TABLET_NAME, LanguageTablet, Translation } from './tablets/tablets'; import { Translator } from './translate'; import { printDiagnostics } from './util'; +export enum UnknownSnippetMode { + /** + * Return the snippet as given (untranslated) + */ + VERBATIM = 'verbatim', + + /** + * Live-translate the snippet as best as we can + */ + TRANSLATE = 'translate', + + /** + * Throw an error if this occurs + */ + FAIL = 'fail', +} + export interface RosettaOptions { /** * Whether or not to live-convert samples * - * @default false + * @default UnknownSnippetMode.VERBATIM */ - readonly liveConversion?: boolean; + readonly unknownSnippets?: UnknownSnippetMode; /** * Target languages to use for live conversion @@ -75,9 +93,11 @@ export class Rosetta { private readonly extractedSnippets = new Map(); private readonly translator: Translator; private readonly loose: boolean; + private readonly unknownSnippets: UnknownSnippetMode; public constructor(private readonly options: RosettaOptions = {}) { this.loose = !!options.loose; + this.unknownSnippets = options.unknownSnippets ?? UnknownSnippetMode.VERBATIM; this.translator = new Translator(options.includeCompilerDiagnostics ?? false); } @@ -115,7 +135,7 @@ export class Rosetta { * * Otherwise, if live conversion is enabled, the snippets in the assembly * become available for live translation later. This is necessary because we probably - * need to fixture snippets for successful compilation, and the information + * need to fixturize snippets for successful compilation, and the information * pacmak sends our way later on is not going to be enough to do that. */ public async addAssembly(assembly: spec.Assembly, assemblyDir: string) { @@ -129,9 +149,11 @@ export class Rosetta { } } - if (this.options.liveConversion) { + // Inventarize the snippets from this assembly, but only if there's a chance + // we're going to need them. + if (this.unknownSnippets === UnknownSnippetMode.TRANSLATE) { for (const tsnip of allTypeScriptSnippets([{ assembly, directory: assemblyDir }], this.loose)) { - this.extractedSnippets.set(tsnip.visibleSource, tsnip); + this.extractedSnippets.set(snippetKey(tsnip), tsnip); } } } @@ -147,9 +169,16 @@ export class Rosetta { * will be based on the snippet key, which consists of a hash of the * visible source and the API location. * - Otherwise, translate the snippet as-is (without fixture information). + * + * This will do and store a full conversion of the given snippet, even if it only + * returns one language. Subsequent retrievals for the same snippet in other + * languages will reuse the translation from cache. + * + * If you are calling this for the side effect of adding translations to the live + * tablet, you only need to do that for one language. */ public translateSnippet(source: TypeScriptSnippet, targetLang: TargetLanguage): Translation | undefined { - // Look for it in loaded tablets + // Look for it in loaded tablets (or previous conversions) for (const tab of this.allTablets) { const ret = tab.lookup(source, targetLang); if (ret !== undefined) { @@ -157,9 +186,23 @@ export class Rosetta { } } - if (!this.options.liveConversion) { - return undefined; + if (this.unknownSnippets === UnknownSnippetMode.VERBATIM) { + return { + language: targetLang, + source: source.visibleSource, + }; + } + + if (this.unknownSnippets === UnknownSnippetMode.FAIL) { + const message = [ + 'The following snippet was not found in any of the loaded tablets:', + source.visibleSource, + `Location: ${JSON.stringify(source.location)}`, + `Language: ${targetLang}`, + ].join('\n'); + throw new Error(message); } + if (this.options.targetLanguages && !this.options.targetLanguages.includes(targetLang)) { throw new Error( `Rosetta configured for live conversion to ${this.options.targetLanguages.join( @@ -168,15 +211,16 @@ export class Rosetta { ); } - // See if we're going to live-convert it with full source information - const extracted = this.extractedSnippets.get(source.visibleSource); + // See if we can find a fixturized version of this snippet. If so, use that do the live + // conversion. + const extracted = this.extractedSnippets.get(snippetKey(source)); if (extracted !== undefined) { const snippet = this.translator.translate(extracted, this.options.targetLanguages); this.liveTablet.addSnippet(snippet); return snippet.get(targetLang); } - // Try to live-convert it on the spot (we won't have "where" information or fixtures) + // Try to live-convert it as-is. const snippet = this.translator.translate(source, this.options.targetLanguages); this.liveTablet.addSnippet(snippet); return snippet.get(targetLang); diff --git a/packages/jsii-rosetta/test/rosetta.test.ts b/packages/jsii-rosetta/test/rosetta.test.ts index a2d58e332a..596037eaa6 100644 --- a/packages/jsii-rosetta/test/rosetta.test.ts +++ b/packages/jsii-rosetta/test/rosetta.test.ts @@ -7,6 +7,7 @@ import { TypeScriptSnippet, DEFAULT_TABLET_NAME, Translation, + UnknownSnippetMode, } from '../lib'; import { TargetLanguage } from '../lib/languages'; import { fakeAssembly } from './jsii/fake-assembly'; @@ -23,7 +24,7 @@ describe('Rosetta object can do live translation', () => { beforeEach(() => { // GIVEN rosetta = new Rosetta({ - liveConversion: true, + unknownSnippets: UnknownSnippetMode.TRANSLATE, targetLanguages: [TargetLanguage.PYTHON], }); @@ -70,7 +71,7 @@ test('Can use preloaded tablet', () => { test('Rosetta object can do live translation', () => { // GIVEN const rosetta = new Rosetta({ - liveConversion: true, + unknownSnippets: UnknownSnippetMode.TRANSLATE, targetLanguages: [TargetLanguage.PYTHON], }); @@ -84,10 +85,36 @@ test('Rosetta object can do live translation', () => { }); }); +test('Rosetta object can fail on untranslated snippet', () => { + // GIVEN + const rosetta = new Rosetta({ + unknownSnippets: UnknownSnippetMode.FAIL, + targetLanguages: [TargetLanguage.PYTHON], + }); + + // WHEN + expect(() => { + rosetta.translateSnippet(SAMPLE_CODE, TargetLanguage.PYTHON); + }).toThrow(/snippet was not found/); +}); + +test('Rosetta can give you an untranslated snippet back', () => { + // GIVEN + const rosetta = new Rosetta({ + unknownSnippets: UnknownSnippetMode.VERBATIM, + targetLanguages: [TargetLanguage.PYTHON], + }); + + // WHEN + const translated = rosetta.translateSnippet(SAMPLE_CODE, TargetLanguage.PYTHON); + + expect(translated?.source).toEqual('callThisFunction();'); +}); + test('Rosetta object can do translation and annotation of snippets in MarkDown', () => { // GIVEN const rosetta = new Rosetta({ - liveConversion: true, + unknownSnippets: UnknownSnippetMode.TRANSLATE, targetLanguages: [TargetLanguage.PYTHON], });