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

Document or lint as much as possible of our code style #2604

Open
IvanGoncharov opened this issue Jun 3, 2020 · 5 comments
Open

Document or lint as much as possible of our code style #2604

IvanGoncharov opened this issue Jun 3, 2020 · 5 comments
Milestone

Comments

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 3, 2020

We have a pretty uniform codebase which is very important for the reference implementation.
That said I the price that we pay for this it's extremely hard to start contributing since you get a huge amount of review comments in your first PR.
Also, it gave the wrong incentives to maintainers (including myself) to just rewriting PR from scratch since it was way easier than to explain what's wrong with the code.

Don't think that there is any silver bullet but we can do a couple of things:

  • Using existing linters and plugins for them (that said we don't want to be dependent on projects without big and active community).
  • Write custom ESLint rules for stuff that can be easily checked in JS AST
  • Expand our CONTRIBUTION.md
  • Investigate alternative tooling (e.g. Jest inline snapshots should solve issues with consistent formatting of JSON blobs in tests).

I think a good strategy would be to first work on common issues that most of the first-time contributors struggling with:

  • order of imports
  • keeping Flow and TS in sync (it would be an issue even after TS migration since we still need to keep Flow typing in sync).
  • properly export public APIs from main index.js and one of the top level folders
  • uniformity of JSON blobs in our tests
  • To be added
@IvanGoncharov
Copy link
Member Author

  • forbid to export index files in our source code and tests
  • add note that it is totally ok to fix stuff unrelated to the main point of PR but it should be done in separate PRs

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Jun 5, 2020

  • add a note that every workaround should be documented with FIXME and stuff that we decided to implement latter with TODO ideally they should contain a link to either GitHub issue or link to PR comment.

@IvanGoncharov
Copy link
Member Author

  • add a general note that you should always match the style of the file that you are working in
  • add a note that you shouldn't blindly add stuff at the end of file, array, object, etc. but instead should try to understand its placement logic and try to fit your stuff.

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Jun 5, 2020

  • note that you should self-review your PRs (except RFC PRs). Good exercise is to run git add -p or review diff that GitHub shows you before you create PR. You should be able to explain to yourself every change that you made. A good reviewer will do the same but by doing it yourself you will save a lot of time for both parties.

@IvanGoncharov IvanGoncharov added this to the post-16.0.0 milestone Aug 13, 2020
@IvanGoncharov
Copy link
Member Author

  • General rule of thumb that we try to stick to official packages and not depend on packages that are not widely used. It’s done both for security and minimise long-term maintenance effort.
  • Before adding new 3rd-party dependency it's preferable try to write a small (up to couple dozen lines) function instead, if it's possible.
  • This package has zero runtime dependencies and it should stay the same since it mean code is this library is easily portable on other languages/platforms. Also it allow us to support more platforms/environments since this package doesn't need anything beyond standard JS enviroment.

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

1 participant