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

Migrate JSON.parse() to parseJson() calls #36

Closed
wants to merge 2 commits into from

Conversation

jpabeem
Copy link

@jpabeem jpabeem commented Oct 29, 2021

Ref #35

@jy95 can you check this PR?

@jpabeem jpabeem marked this pull request as draft October 29, 2021 09:13
@jpabeem jpabeem marked this pull request as ready for review October 29, 2021 09:14
@jy95
Copy link
Owner

jy95 commented Oct 29, 2021

Hello @jpabeem,

Thanks for your PR (and also for the missing pull_request event for the automated test workflow 🤦‍♂️).
If you don't mind, I see one possible improvement : when possible, add the filename as well in parseJson() calls

Such as :

import path from 'path';
const filename = path.basename("/foo/bar/fr.json");
parseJson('{\n\t"foo": true,\n}', filename);

@@ -60,7 +61,7 @@ async function verify_files_entry([_, i18nPath]: [string, any]): Promise<
}
// check if the file is a JSON
try {
JSON.parse(potentialJSON.toString());
parseJson(potentialJSON.toString());
return Promise.resolve(true);
} catch (error) {
return Promise.reject(new Error(`${i18nPath} isn't a valid JSON`));
Copy link
Owner

Choose a reason for hiding this comment

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

Promise.reject(error) instead as parseJson raises a JSONError if an issue arose

@@ -18,7 +20,7 @@ export default class CommandBuilder {
let ext = extname(configPath);
return /\.js$/i.test(ext)
? require(configPath)
: JSON.parse(readFileSync(configPath, 'utf-8'));
: parseJson(readFileSync(configPath, 'utf-8'));
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested improvement :

const filename = path.basename(configPath);
parseJson(readFileSync(configPath, 'utf-8'), filename);

@jy95 jy95 added the hacktoberfest For Hacktoberfest label Nov 20, 2021
@jy95 jy95 closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest For Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants