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 --fix CLI option #367

Merged
merged 3 commits into from
Jun 28, 2021
Merged

Add --fix CLI option #367

merged 3 commits into from
Jun 28, 2021

Conversation

Mikek2252
Copy link
Collaborator

@Mikek2252 Mikek2252 commented Apr 26, 2021

This PR add a --fix option when running ember-intl-analyzer that should remove all unused translations whether they are whole keys or nested.

This is the first draft

Todo:

  • update documentation to show the --fix option

@Mikek2252 Mikek2252 requested a review from a team April 26, 2021 15:55
@Turbo87 Turbo87 changed the title Fix option Add --fix CLI option Apr 29, 2021
test.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@Mikek2252 Mikek2252 force-pushed the fix-option branch 2 times, most recently from 9d26824 to ba2121a Compare May 24, 2021 10:38
index.js Outdated
@@ -55,6 +57,7 @@ async function run(rootDir, options = {}) {
log(' 👏 No unused translations were found!');
} else {
log(` ⚠️ Found ${chalk.bold.yellow(unusedTranslations.size)} unused translations!`);
log(' use --fix to remove all unused translations');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log(' use --fix to remove all unused translations');
log(' You can use --fix to remove all unused translations.');

so that this aligns a little better :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

and I guess we should only log this line if shouldFix is not true?

index.js Outdated
@@ -13,6 +13,8 @@ const YAML = require('yaml');

async function run(rootDir, options = {}) {
let log = options.log || console.log;
let writeToFile = options.writeToFile || fs.writeFileSync;
let shouldFix = process.argv.includes('--fix');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to pass fix: true/false in as one of the options. That way we don't rely on the global process.argv state and this run() function stays reusable. It also means you don't have to use the process.argv.push('--fix') hack below 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

like this?

Suggested change
let shouldFix = process.argv.includes('--fix');
let shouldFix = options.fix || process.argv.includes('--fix');

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking more like this:

Suggested change
let shouldFix = process.argv.includes('--fix');
let shouldFix = options.fix ?? false;

and then in bin/cli.js we use run(rootDir, { fix: process.argv.includes('--fix') })

@Turbo87 Turbo87 added the enhancement New feature or request label May 27, 2021
Copy link
Collaborator

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

👍

@Mikek2252 Mikek2252 merged commit 46b6d56 into master Jun 28, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix-option branch June 28, 2021 13:42
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