-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add support for listing redundant whitelist entries #649
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{{t "some.present.translation"}} | ||
{{t "some.whitelisted.translation-a"}} | ||
{{t "some.whitelisted.translation-b"}} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
some: | ||
present: | ||
translation: This translation exists |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,21 +67,30 @@ async function run(rootDir, options = {}) { | |
existingExternalTranslationKeys | ||
); | ||
let whitelist = config.whitelist || []; | ||
let usedWhitelistEntries = new Set(); | ||
let errorOnUnusedWhitelistEntries = config.errorOnUnusedWhitelistEntries || false; | ||
|
||
let unusedTranslations = findDifferenceInTranslations( | ||
existingOwnTranslationKeys, | ||
usedTranslationKeys, | ||
whitelist | ||
whitelist, | ||
usedWhitelistEntries | ||
); | ||
|
||
log(`${step(4)} ⚙️ Checking for missing translations...`); | ||
log(); | ||
let missingTranslations = findDifferenceInTranslations( | ||
usedTranslationKeys, | ||
existingTranslationKeys, | ||
whitelist | ||
whitelist, | ||
usedWhitelistEntries | ||
); | ||
|
||
let unusedWhitelistEntries = new Set(whitelist); | ||
for (let entry of usedWhitelistEntries) { | ||
unusedWhitelistEntries.delete(entry); | ||
} | ||
|
||
if (unusedTranslations.size === 0) { | ||
log(' 👏 No unused translations were found!'); | ||
} else { | ||
|
@@ -106,7 +115,23 @@ async function run(rootDir, options = {}) { | |
} | ||
} | ||
|
||
let totalErrors = missingTranslations.size + unusedTranslations.size; | ||
if (unusedWhitelistEntries.size > 0) { | ||
log(); | ||
log( | ||
` ⚠️ Found ${chalk.bold.yellow(unusedWhitelistEntries.size)} unused whitelist ${ | ||
unusedWhitelistEntries.size === 1 ? 'entry' : 'entries' | ||
}! Please remove ${unusedWhitelistEntries.size === 1 ? 'it' : 'them'} from the whitelist:` | ||
); | ||
log(); | ||
for (let entry of unusedWhitelistEntries) { | ||
log(` - ${entry}`); | ||
} | ||
} | ||
|
||
let totalErrors = | ||
missingTranslations.size + | ||
unusedTranslations.size + | ||
(errorOnUnusedWhitelistEntries ? unusedWhitelistEntries.size : 0); | ||
|
||
if (shouldFix) { | ||
removeUnusedTranslations(writeToFile, rootDir, ownTranslationFiles, unusedTranslations); | ||
|
@@ -421,15 +446,21 @@ function forEachTranslation(json, callback, prefix = '') { | |
|
||
// Find all translation keys that appear in translation map A, but not | ||
// in translation map B | ||
function findDifferenceInTranslations(mapA, mapB, whitelist) { | ||
function findDifferenceInTranslations(mapA, mapB, whitelist, usedWhitelistEntries) { | ||
let missingTranslations = new Map(); | ||
|
||
for (let [key, files] of mapA) { | ||
const keyTrimmed = key.trim(); | ||
const isKeyMissing = !mapB.has(keyTrimmed); | ||
const isKeyAllowed = !whitelist.some(regex => regex.test(keyTrimmed)); | ||
|
||
if (isKeyMissing && isKeyAllowed) { | ||
if (!isKeyMissing) continue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Little performance improvement here; if the key wasn't missing, it would still be looked up in the whitelist, which is useless. |
||
|
||
const whitelistKey = whitelist.find(regex => regex.test(keyTrimmed)); | ||
const isKeyWhitelisted = whitelistKey != null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it easier to follow when called |
||
|
||
if (isKeyWhitelisted) { | ||
usedWhitelistEntries.add(whitelistKey); | ||
} else { | ||
missingTranslations.set(keyTrimmed, files); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's nice to make the exit code dependent on unused whitelist entries as well. However, I chose to leave this as a config option, so that current users aren't affected by this change. Alternative is just not having this config option and still error, but this might block applications after upgrading. Also, if this feature contains a bug that I overlooked, it's easy to turn it off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will keep it opt in for now, and see how it goes and that it is working well, then i think for the next Major version i think i will have this on by default.