Beautify settings files on migration #323
Conversation
| @@ -4,7 +4,7 @@ import {join} from 'path'; | |||
| import {prompt} from 'inquirer'; | |||
| import {green, red, yellow} from 'chalk'; | |||
| import figures from 'figures'; | |||
| import {downloadFromUrl, startProcess, writePackageJsonSync, move, isShopifyTheme, isShopifyThemeWhitelistedDir} from '../utils'; | |||
| import {downloadFromUrl, startProcess, writePackageJsonSync, move, beautifyJson, isShopifyTheme, isShopifyThemeSettingsFile, isShopifyThemeWhitelistedDir} from '../utils'; | |||
There was a problem hiding this comment.
I feel like this declaration is overkill -- what's your opinion on importing the whole utils object and call the methods from that?
packages/slate-cli/src/utils.js
Outdated
| * @param {string} directory - The path to the directory. | ||
| */ | ||
| export function isShopifyThemeSettingsFile(file) { | ||
| const whitelist = ['settings_schema.json', 'settings_data.json']; |
There was a problem hiding this comment.
These values are buried pretty deep in the code. Makes me think this package could use a config file. Definitely not needed in this PR though.
packages/slate-cli/src/utils.js
Outdated
| * | ||
| * @param {string} file - The path to the file. | ||
| */ | ||
| export function beautifyJson(file) { |
There was a problem hiding this comment.
I find "beautify" a bit generic - thoughts on unminifyJson instead?
There was a problem hiding this comment.
I feel like unminifyJson assumes that the file is minified — which it might not be — though I don't hold to that opinion very strongly. unminifyJson works for me.
| @@ -35,6 +35,7 @@ export default function(program) { | |||
| const configYml = join(workingDirectory, 'config.yml'); | |||
| const pkgJson = join(workingDirectory, 'package.json'); | |||
| const srcDir = join(workingDirectory, 'src'); | |||
| const srcConfigDir = join(workingDirectory, 'src/config'); | |||
There was a problem hiding this comment.
Would rename to configDir.
|
|
||
| await Promise.all(beautifyPromises); | ||
|
|
||
| console.log(` ${green(figures.tick)} Beautification of settings files completed`); |
There was a problem hiding this comment.
This can definitely be part of the migration task but do we need to output this "beautification" message to the user? Might lead to confusion. Perhaps it can run behind the scenes and worse case it fails - where the error would then be caught as part of the catch block.
@t-kelly @maximevaillancourt thoughts?
| @@ -4,7 +4,7 @@ import {join} from 'path'; | |||
| import {prompt} from 'inquirer'; | |||
| import {green, red, yellow} from 'chalk'; | |||
| import figures from 'figures'; | |||
| import {downloadFromUrl, startProcess, writePackageJsonSync, move, isShopifyTheme, isShopifyThemeWhitelistedDir} from '../utils'; | |||
| import {downloadFromUrl, startProcess, writePackageJsonSync, move, beautifyJson, isShopifyTheme, isShopifyThemeSettingsFile, isShopifyThemeWhitelistedDir} from '../utils'; | |||
packages/slate-cli/src/utils.js
Outdated
| * @param {string} file - The path to the file. | ||
| */ | ||
| export function isShopifyThemeSettingsFile(file) { | ||
| const whitelist = ['settings_schema.json', 'settings_data.json']; |
There was a problem hiding this comment.
Maybe for future proof, we can use a Regex to check whether or not the format is settings_*.json instead of having a hardcoded whitelist.
There was a problem hiding this comment.
Could be as simple as that:
export function isShopifyThemeSettingsFile(file) {
return /^settings_.+\.json/.test(file);
}
| const movePromises = whitelistFiles.map(movePromiseFactory); | ||
|
|
||
| function beautifyJsonPromiseFactory(file) { | ||
| console.log(` Beautifying ${file}...`); |
|
This PR is good for another round of review. @chrisberthe @t-kelly |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
What are you trying to accomplish with this PR?
Beautify the settings files (
settings_*.json) upon migration (fix #224).Expected results
The settings files are pretty printed (indented with 2 spaces).
Checklist
For contributors:
For maintainers: