-
Notifications
You must be signed in to change notification settings - Fork 4
Setup Prettier to format code #308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to review as the GH UI almost crashes, but managed to post few comments. In general the code style is tough question.
let key = type | ||
let _ | ||
if (isObject(key)) { | ||
[[key, _]] = Object.entries(key) | ||
;[[key, _]] = Object.entries(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/ApiEncoder.js
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely disagree with this extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/BytecodeTypeResolver.js
Outdated
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with forcing non-mandatory () here, mainly because of readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely prefer multiline list both for readability and maintenance regardless if it fits on line limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, single line should be also allowed as it works better in some code blocks/snippets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
]
src/DataFactory/RecordDataFactory.js
Outdated
FateTypeRecord(type.keys, type.valueTypes), | ||
resolvedValue | ||
) | ||
return new FateTuple(FateTypeRecord(type.keys, type.valueTypes), resolvedValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single line shouldn't be enforced in this case as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no good way to implement this: we can reduce printWidth
or add a comment as the above 🤷♀️
src/DataFactory/TupleDataFactory.js
Outdated
@@ -9,10 +9,7 @@ class TupleDataFactory extends BaseDataFactory { | |||
|
|||
create(type, value) { | |||
if (!Array.isArray(value)) { | |||
throw new FateTypeError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, enforced single line which I don't like
prettier.cjs
Outdated
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to rewrite files on the fly to prevent array and fn call collapsing, and this seems to be working
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