Check formatting #5760
Check formatting #5760
Conversation
tsnobip
force-pushed
the
check_formatting
branch
from
799f01b to
ad22c1f
Compare
scripts/rescript_format.js
Outdated
| hasError = true; | ||
| var incorrectlyFormattedFiles = 0; | ||
| try { | ||
| const _promises = await Promise.all( |
There was a problem hiding this comment.
I noticed - and this applies to the old as well as the new implementation - that bsc -format is run in parallel for all files.
Not sure how far this will scale. Maybe this should be changed to processing in batches, but that's probably out of the scope of this PR?
There was a problem hiding this comment.
I'd say it's indeed out of scope of this PR, especially if we don't have any report of issues caused by this.
scripts/rescript_format.js
Outdated
| if (!write) { | ||
| process.stdout.write(stdout); | ||
| try { | ||
| const formatted = child_process.execFileSync(bsc_exe, flags); |
There was a problem hiding this comment.
As discussed on Discord, please add a comment explaining why processing is sync here, but async above.
There was a problem hiding this comment.
Side comment: do we have a discord community channel? 😲
There was a problem hiding this comment.
@mattdamon108 haha no dm
There was a problem hiding this comment.
@mattdamon108 What's your Discord handle?
There was a problem hiding this comment.
As discussed on Discord, please add a comment explaining why processing is sync here, but async above.
now that I think about it, I don't know if the asynchronous processing we had before actually created any issue? I'll indeed add some comments about my reasoning.
There was a problem hiding this comment.
I rewrote it to use the same logic, there's no reason why the output would be mixed anyway.
|
Ah, I forgot: @tsnobip could you also add an entry to the changelog? |
tsnobip
force-pushed
the
check_formatting
branch
from
8b9d3b9 to
19871aa
Compare
Fixes #4780.
This PR also fixes a few bugs:
rescript formatusage was not being displayed