Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upmaintain: const over let, consistency #5494
Conversation
|
I have updated some files to be consistent and written in "perfect ES6 style". |
This comment has been minimized.
This comment has been minimized.
mshirlaw
commented on lib/build.js in 281fc57
Oct 4, 2019
•
|
This is potentially reassigned three lines below isn't it? |
This comment has been minimized.
This comment has been minimized.
mshirlaw
commented on lib/build.js in 281fc57
Oct 4, 2019
•
|
Previously this returned truthy only when path.ext(file) was .icon but now it returns true whenever the return value of path.extname(file) returns truthy. Isn't this a behavioral change? The comment implies it should only return icon files. |
This comment has been minimized.
This comment has been minimized.
mshirlaw
commented on scripts/lint.py in 281fc57
Oct 4, 2019
|
Is there a reason why you would include python whitespace changes in a PR that is cleaning up const and let in JavaScript files? Seems dangerous to me |
This comment has been minimized.
This comment has been minimized.
mshirlaw
commented on scripts/lint.py in 281fc57
Oct 4, 2019
|
Including function arguments on a new line is valid python and even enforced by many linters. Is this change required? |
| const ext = path.extname(file) | ||
| if (ext !== '.icon') { return false } | ||
| return true | ||
| return path.extname(file) === true |
This comment has been minimized.
This comment has been minimized.
|
|
||
| // Touch original files by updating mtime. | ||
| const chromiumSrcDirLen = chromiumSrcDir.length | ||
| sourceFiles.forEach(chromiumSrcFile => { | ||
| var overriddenFile = path.join(config.srcDir, chromiumSrcFile.slice(chromiumSrcDirLen)) | ||
| const overriddenFile = path.join(config.srcDir, chromiumSrcFile.slice(chromiumSrcDirLen)) | ||
| if (!fs.existsSync(overriddenFile)) { | ||
| // Try to check that original file is in gen dir. | ||
| overriddenFile = path.join(config.outputDir, 'gen', chromiumSrcFile.slice(chromiumSrcDirLen)) |
This comment has been minimized.
This comment has been minimized.
|
adapted @mshirlaw and @spirinvladimir suggestions. Reverted changes in python - though there shouldn't be any difference in the file functionality. Especially in that indent change in the last line. But agree - JS files are JS files. |
| @@ -2,6 +2,7 @@ const config = require('../lib/config') | |||
| const util = require('../lib/util') | |||
| const {braveTopLevelPaths, ethereumRemoteClientPaths} = require('./l10nUtil') | |||
|
|
|||
|
|
|||
This comment has been minimized.
This comment has been minimized.
|
Applied @petemill's suggestion and merged master (please check out the second commit if I merged correctly) |
kmdrGroch commentedAug 1, 2019
Reviewer Checklist:
After-merge Checklist:
changes has landed on.