search

near / borsh-js

TypeScript/JavaScript implementation of Binary Object Representation Serializer for Hashing
Apache License 2.0
112 stars 38 forks source link

Add Prettier to keep code consistent. #33

Closed tbezman closed 3 years ago

tbezman commented 3 years ago

Hoping this a welcome addition. I added prettier and setup a config file that closely matches the existing code style that already existed. I'm also only checking index.ts for now which seems like one of our only TypeScript source files.

I also setup the repo's first Github Action to ensure code is properly formatted from people posting PRs.

volovyks commented 3 years ago

Wow, Awesome contribution @tbezman ! We definitely need to add GitHub actions here. However, we are already using eslint across our projects. We have lint and fix commands in Borsh JS package.json. eslint is a linter, so it's more powerful and can look for potential errors in code. But I can agree that Prettier is doing a better job in code formatting. So I can see two ways how we can stay consistent and make this project better.

  1. (Easy) Use eslint instead of Prettier in added GitHub Action.
  2. (Hard) Use both eslint and Prettier. They can be in conflict, so we will need to read this article first. Maybe we will need to have two separate actions for them.

I would rather use default parameters for Prettier and not override them with a separate config file. More chances that it will not be in conflict with text editors and IDEs.

Bonus points for adding Unit tests GitHub Action (just run yarn test)

tbezman commented 3 years ago

Thanks for getting back to me. I'll apply all of this feedback ASAP.

tbezman commented 3 years ago

@volovyk-s Yeah there is a config we can use to disable all eslint rules around code formatting so we can use prettier for formatting, and eslint for code conventions. I think I'll save the ESLint configuration for an upcoming PR since we have a bunch of warnings atm. I'll do the eslint stuff as a followup.

tbezman commented 3 years ago

Okay @volovyk-s, I updated the PR. I removed the prettier config (now we're using prettier defaults), and added unit tests to the github action workflow. I think you'll have to make the action a required check on PRs after this merges.

tbezman commented 3 years ago

@volovyk-s On this branch, when I run yarn pretty, none of my files actually change but I do get some output in the console about which files prettier ran against. Are you seeing the same behavior or is something borked?

volovyks commented 3 years ago

@tbezman I have double-checked. All good, thank you!

volovyks commented 3 years ago

Hm, it's getting changed after yarn test, that is weird :/

volovyks commented 3 years ago

Test and pretty (with small fix) GitHub actions works! https://github.com/near/borsh-js/pull/35