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

Add support for listing redundant whitelist entries #649

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

robinborst95
Copy link
Contributor

@robinborst95 robinborst95 commented Dec 20, 2023

Implements #645.

@robinborst95 robinborst95 force-pushed the feature/unused-whitelist branch 2 times, most recently from 18ee8bf to 50a7430 Compare January 2, 2024 18:09
@@ -48,6 +48,19 @@ To prevent that from happening you can configure a `whitelist`, which accepts an
array of regular expressions that will be checked when looking for unused
translations.

### `errorOnUnusedWhitelistEntries`
Copy link
Contributor Author

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.

Copy link
Collaborator

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.


if (isKeyMissing && isKeyAllowed) {
if (!isKeyMissing) continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

if (!isKeyMissing) continue;

const whitelistKey = whitelist.find(regex => regex.test(keyTrimmed));
const isKeyWhitelisted = whitelistKey != null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it easier to follow when called isKeyWhitelisted as opposed to isKeyAllowed

@robinborst95 robinborst95 marked this pull request as ready for review January 2, 2024 19:02
@Mikek2252 Mikek2252 added the enhancement New feature or request label Feb 2, 2024
@Mikek2252 Mikek2252 force-pushed the feature/unused-whitelist branch from f74de25 to c150f43 Compare February 2, 2024 10:43
@Mikek2252 Mikek2252 merged commit 46a3e6b into mainmatter:master Feb 2, 2024
5 checks passed
@robinborst95 robinborst95 deleted the feature/unused-whitelist branch February 2, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants