Skip to content
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

Enforce code style? #83

Open
ngbrown opened this issue Aug 10, 2023 · 5 comments
Open

Enforce code style? #83

ngbrown opened this issue Aug 10, 2023 · 5 comments

Comments

@ngbrown
Copy link
Contributor

ngbrown commented Aug 10, 2023

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
Copy link
Contributor Author

ngbrown commented Aug 11, 2023

This is what these two options could look like:

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.

@dentarg
Copy link
Member

dentarg commented Aug 11, 2023

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?

@carlhoerberg
Copy link
Member

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

@carlhoerberg
Copy link
Member

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

@ngbrown
Copy link
Contributor Author

ngbrown commented Aug 11, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants