amqp-client.js icon indicating copy to clipboard operation
amqp-client.js copied to clipboard

Enforce code style?

Open ngbrown opened this issue 2 years ago • 5 comments

As I have been making contributions, I try to match the surrounding code style, but sometimes add semicolons, or get the single-quote/double-quote different.

Prettier would make it easy to enforce a single code style for everyone.

There are currently 423 single-quotes and 870 double-quotes in the source files.

Likewise, there are 428 semicolons, but mostly in multi-statement lines like these:

closeOk.setUint8(j, 1); j += 1 // type: method
closeOk.setUint16(j, channelId); j += 2 // channel
closeOk.setUint32(j, 4); j += 4 // frameSize

The biggest change is that Prettier will force the statements above onto separate lines.

Considering the above, these are the options that could be used:

{
  "semi": false,
  "singleQuote": false // default is `false`
}

With "semi": false, that runs into a potential linting issue because Prettier is trying to prevent a potential ASI failure with changes like this:

case "S":
  ;[v, len] = this.getLongString(i, littleEndian)
  i += len
  break

ESLint will complain about unnecessary semicolons (but can be disabled with eslint-config-prettier)... Maybe semicolons are fine after all?

Alternatively ESLint could be used to enforce a coding style, starting with the semi and quotes rules. The full list of rules that would need to be configured is extensive.

Using linters for formatting is not really recommended, including by ESLint.

ngbrown avatar Aug 10 '23 17:08 ngbrown

This is what these two options could look like:

  • Prettier: https://github.com/cloudamqp/amqp-client.js/compare/main...ngbrown-forks:amqp-client.js:feature/prettier
  • ESLint: https://github.com/cloudamqp/amqp-client.js/compare/main...ngbrown-forks:amqp-client.js:feature/eslint-format-rules

I separated the ESLint branch commits by fixing each rule in turn, but the fixes would be squashed into a single commit for a pull request.

Dealing with manually ensuring the format is correct is way more annoying than just letting Prettier do it automatically on every editor save. I included Prettier editor configurations for both VS Code and WebStorm.

Prettier can also take care of markdown files and code inside of markdown files.

ngbrown avatar Aug 11 '23 01:08 ngbrown

I'm in favour of this 👍 I would go with Prettier, as it to me doesn't make sense to go against the recommendations from TypeScript.

@carlhoerberg do you have any opinions on this?

dentarg avatar Aug 11 '23 06:08 dentarg

Breaking up closeOk.setUint32(j, 4); j += 4 // frameSize to three lines does not look good.

carlhoerberg avatar Aug 11 '23 06:08 carlhoerberg

Not really excited about 1300 more lines in the code base.. but on my phone and cant make a just assessment yet.

carlhoerberg avatar Aug 11 '23 07:08 carlhoerberg

There's a couple options I can think of,

Get the comment back onto the function call:

closeOk.setUint32(j, 4) // frameSize
j += 4

Use a postfix increment function and object and get it all back on one line:

let j = new Position(0) // position in outgoing frame
// and
closeOk.setUint32(j.poInc(4), 4) // frameSize

elsewhere:

export class Position {
  public position: number
  constructor(initialPosition = 0) {
    this.position = initialPosition
  }
  /** Post increment by offset, returning position before incrementing. */
  poInc(offset = 1) {
    const p = this.position
    this.position += offset
    return p
  }
}

Ignore sections of code

// prettier-ignore can be used to ignore blocks of code, so for the three files with this pattern, ignore each function. But this will lose the benefits of Prettier in those entire functions.

ngbrown avatar Aug 11 '23 08:08 ngbrown