refactor(cli): print relevant values when comparing fails #585
Conversation
|
@gurrpi what's the status on this? Do you think this is worth finishing/merging? Or should we close this out? |
I've just triggered a new CI run. Once the checks pass, I think this is worth merging. |
|
|
Nudge on this @baumstern |
baumstern
force-pushed
the
print
branch
2 times, most recently
from
909d4e2 to
639f561
Compare
|
@ctrlc03 PTAL |
cli/ts/utils.ts
Outdated
| @@ -142,6 +142,21 @@ const isPathExist = (paths: Array<string>): [boolean, string] => { | |||
| return [true, null] | |||
| } | |||
|
|
|||
| const CompareOnchainValue = ( | |||
There was a problem hiding this comment.
| const CompareOnchainValue = ( | |
| const compareOnChainValue = ( |
General convention - functions should always be camelCase, ya?
Noting this commit suggestion would require updating all uses of this function...
There was a problem hiding this comment.
Edit - also capitalized the "C". See comment below for context
There was a problem hiding this comment.
General convention - functions should always be camelCase, ya?
Noting this commit suggestion would require updating all uses of this function...
Good catch! Reflected in 974ab46
|
@baumstern any particular reason you PR'd from your fork vs. creating a branch directly off the repo? |
cli/ts/proveOnChain.ts
Outdated
| @@ -380,6 +375,7 @@ const proveOnChain = async (args: any) => { | |||
| return | |||
| } | |||
|
|
|||
| const txErr = 'Error: processMessages() failed' | |||
There was a problem hiding this comment.
Another consistency nitpick - looks like you're using a mix of single quotes & double quotes. Do we have a convention on this? I don't have a strong preference... just that we be consistent
cli/ts/utils.ts
Outdated
| ): boolean => { | ||
| if (onChainValue !== offChainValue) { | ||
| console.error(`Error: ${name} mismatch.`) | ||
| console.error(" onChainValue: " + onChainValue) |
There was a problem hiding this comment.
Why the spacing on these logs?
There was a problem hiding this comment.
The spacing in these logs is intended to make the output more noticeable and easier to read.
I think I didn't dare to add a branch to the upstream repo at that moment 😂 |
The inconsistency has been inherited from legacy code. I believe it would be better to address the codebase's overall consistency, so I have filed separate issues to tackle this: |
I'm just referring to your changes in this PR - you use both But agree, would be good to unify across the codebase as well 👍 |
That's reasonable. Fixed the inconsitency of this PR in 2e26126 to uses double quotes and Lint errors such as those in #457 should be fixed and automatically enforced linting in pre-commit hook would ensure us to have consistent style after that. |
Better Logging
Before: only error message has been printed
After: error message and relevant values will be printed