Conversation
dincho
left a comment
dincho
left a comment
There was a problem hiding this comment.
Hard to review as the GH UI almost crashes, but managed to post few comments. In general the code style is tough question.
| let _ | ||
| if (isObject(key)) { | ||
| [[key, _]] = Object.entries(key) | ||
| ;[[key, _]] = Object.entries(key) |
There was a problem hiding this comment.
I also didn't expect that 😁 There are edge cases when you are required to add a semicolon. Prettier doesn't detect them accurately. They claim it is made to move lines of code freely: prettier/prettier#1907.
The most popular semicolon-less eslint ruleset also enforces these extra semicolons: https://standardjs.com/rules.html#semicolons
| signature: {tag: 'sg', size: 64, encoder: base58check}, | ||
| commitment: {tag: 'cm', size: 32, encoder: base58check}, | ||
| bytearray: {tag: 'ba', size: 0, encoder: base64check}, | ||
| key_block_hash: { tag: 'kh', size: 32, encoder: base58check }, |
There was a problem hiding this comment.
I definitely disagree with this extra space
There was a problem hiding this comment.
There was no consistency, and I think I've chosen the more popular variant to reduce the diff, but I'll do as you prefer.
| const { symbols } = this._bytecode | ||
|
|
||
| return Object.keys(symbols).find(key => symbols[key] === funName) | ||
| return Object.keys(symbols).find((key) => symbols[key] === funName) |
There was a problem hiding this comment.
I disagree with forcing non-mandatory () here, mainly because of readability
There was a problem hiding this comment.
There was no consistency, and I think I've chosen the more popular variant to reduce the diff, but I'll do as you prefer.
| 'bls12_381.fr', | ||
| 'bls12_381.fp', | ||
| ] | ||
| const TYPES = ['bls12_381.fr', 'bls12_381.fp'] |
There was a problem hiding this comment.
I definitely prefer multiline list both for readability and maintenance regardless if it fits on line limit
There was a problem hiding this comment.
However, single line should be also allowed as it works better in some code blocks/snippets
There was a problem hiding this comment.
Prettier keep line breaks but only in objects: https://prettier.io/docs/rationale#multi-line-objects (and they consider this a bug)
The are only workarounds like prettier-ignore or adding an empty comment 🙁
// prettier-ignore
const TYPES = [ //
'bls12_381.fr',
'bls12_381.fp'
]| FateTypeRecord(type.keys, type.valueTypes), | ||
| resolvedValue | ||
| ) | ||
| return new FateTuple(FateTypeRecord(type.keys, type.valueTypes), resolvedValue) |
There was a problem hiding this comment.
Single line shouldn't be enforced in this case as well
There was a problem hiding this comment.
There is no good way to implement this: we can reduce printWidth or add a comment as the above 🤷♀️
|
|
||
| create(type, value) { | ||
| if (!Array.isArray(value)) { | ||
| throw new FateTypeError( |
There was a problem hiding this comment.
again, enforced single line which I don't like
| // don't collapse function call if multiline | ||
| .replaceAll(/ (\w+)\(\n/g, ` $1(\n${comment}\n`) | ||
| // don't collapse array definition if multiline | ||
| .replaceAll(/ \[\n/g, ` [\n${comment}\n`) |
There was a problem hiding this comment.
I've tried to rewrite files on the fly to prevent array and fn call collapsing, and this seems to be working
2 participants
This PR is supported by the Æternity Foundation
The idea is to use Eslint for sophisticated checks (I'm not sure that it is needed in this project) and Prettier for code formatting since it works faster. I would drop AirBnb rules because they have not been maintained for a couple of years. I also like that Prettier formats JS in MD files.
https://prettier.io/docs/options