Conversation
- refactor files ops funcs - log every step timestamps - add check params existing
- fix tests for changes
|
@yegor256 @maxonfjvipon take a look plz. |
src/files.js
Outdated
| } | ||
| return result; | ||
| } | ||
| module.exports.findFiles = findFiles; |
There was a problem hiding this comment.
@aidarkdev let's use single module.exports at the end of the file
src/commands/normalize.js
Outdated
| const target = path.resolve(opts.target); | ||
| return elapsed(async (tracked) => { | ||
| copyDir(sources, path.join(target, 'before-normalize'), '.eo'); | ||
| const xmirDir = path.join(target, '1-parse'); |
There was a problem hiding this comment.
@aidarkdev let's try to use single named variable as much as possible, parsed instead of xmirDir, xmirs instead of xmirFile, rel instead of relPath, xmir instead of xmirFile and so on. Check this: https://www.yegor256.com/2015/01/12/compound-name-is-code-smell.html
src/commands/normalize.js
Outdated
| console.debug('Found %d XMIR file(s) to normalize', xmirFiles.length); | ||
| for (const xmirFile of xmirFiles) { | ||
| const relPath = path.relative(xmirDir, xmirFile); | ||
| const base = relPath.replace(/\.xmir$/, ''); |
There was a problem hiding this comment.
@aidarkdev I think there's a special function in path module for getting relative path
There was a problem hiding this comment.
@maxonfjvipon you are rigth, there is alredy used relative funct, replace was just redundant code(that cutted file extension), it is deleted for now
src/commands/normalize.js
Outdated
| console.debug('Normalizing %s', relPath); | ||
| let ts = Date.now(); | ||
| const phi = execSync( | ||
| 'phino rewrite --input=xmir --output=phi', |
There was a problem hiding this comment.
@aidarkdev there's no need to call phino a several times. You can just do:
phino rewrite --input=xmir --output=xmir --normalize your-fileThere was a problem hiding this comment.
@maxonfjvipon on first version were debug mode where every phino call result can be saved. Now we can use one singel call, yes. Thanks! fixed
test/commands/test_normalize.js
Outdated
| fs.rmSync(home, {recursive: true, force: true}); | ||
| fs.mkdirSync(source, {recursive: true}); | ||
| fs.mkdirSync(target, {recursive: true}); | ||
| const content = '# sample\n[] > simple\n'; |
There was a problem hiding this comment.
@aidarkdev I see a lot of code duplication across the tests:
const home = path.resolve('temp/test-normalize/simple');
const source = path.resolve(home, 'src');
const target = path.resolve(home, 'target');
fs.rmSync(home, {recursive: true, force: true});
fs.mkdirSync(source, {recursive: true});
fs.mkdirSync(target, {recursive: true});
const content = '# sample\n[] > simple\n';I think it can be moved to some preparation function
The same story with:
runSync([
'normalize',
'--verbose',
`--parser=${parserVersion}`,
`--home-tag=${homeTag}`,
'-s', source,
'-t', target,
]);- single export - single `phino` call on normalize command - cut code duplication on tests
- back tests skipped before for debug - fix link to latest phino binary
|
@maxonfjvipon review comments fixed, all debug logs and other workaround cleaned, checks-tests are ok |
maxonfjvipon
left a comment
There was a problem hiding this comment.
@aidarkdev looks good, just one minor fix and we're good to merge
src/files.js
Outdated
| if (!fs.existsSync(src)) {return;} | ||
| fs.mkdirSync(dst, {recursive: true}); | ||
| for (const entry of fs.readdirSync(src, {withFileTypes: true})) { | ||
| const srcPath = path.join(src, entry.name); |
There was a problem hiding this comment.
@aidarkdev let's use single word name also here source instead of srcPath and dest or destination instead of dstPath
There was a problem hiding this comment.
@maxonfjvipon thanks!
Fixed. All checks-tests are ok.
|
@aidarkdev the only man who can merge the PR is @yegor256. So do be shy, ping him |
|
@yegor256 need to merge here |
|
@yegor256 ping, review ok, need to push here |
|
@yegor256 review ok, waiting merge |
|
@rultor merge |
@aidarkdev @yegor256 Oops, I failed. You can see the full log here (spent 23min). |
|
@yegor256 it seems |
|
@maxonfjvipon @yegor256 opened issue for that |
@yegor256 @maxonfjvipon found out how it works, closed the issue. Added |
|
@aidarkdev thanks! |
Closes #590
Add
normalizecommand. Works via callingphinoutil.