sentry icon indicating copy to clipboard operation
sentry copied to clipboard

ref: remove sentry.lint.engine and get all js linting and formatting into pre-commit

Open joshuarli opened this issue 1 year ago • 2 comments

Cleaned up version of https://github.com/getsentry/sentry/pull/73037.

I was trying very hard to only install what's needed in pre-commit-managed nodeenvs, but wasn't able to get it to work and now I'm opting of just using the entire node_modules (setting up volta + node deps takes 25s on a warm cache).

      # We're using the local node_modules to simplify things, otherwise would
      # need an easy way to keep versions in sync. Furthermore some repositories
      # are either not on pre-commit mirrors or are missing some tagged versions.
      # pre-commit-managed nodeenv environments also do not work out of the box because
      # additional_dependencies does not install a flattened dependency tree which is needed by,
      # for example, our eslint setup.

joshuarli avatar Jun 28 '24 15:06 joshuarli

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.12%. Comparing base (f1a5736) to head (179e087). Report is 3 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #73508       +/-   ##
===========================================
+ Coverage   56.65%   78.12%   +21.46%     
===========================================
  Files        6647     6656        +9     
  Lines      297366   297662      +296     
  Branches    51174    51222       +48     
===========================================
+ Hits       168480   232536    +64056     
+ Misses     124271    58856    -65415     
- Partials     4615     6270     +1655     

see 2019 files with indirect coverage changes

codecov[bot] avatar Jun 28 '24 16:06 codecov[bot]

Does this change mean that we've never running linters/formatters on the entire codebase, since the "all files" steps are gone? If we're not, is that safe? I assume it is, since tsc still runs on everything, but wondering if there are any cases where changing a file might cause a problem in an unchanged file and it won't be linted

gggritso avatar Jul 08 '24 18:07 gggritso

Does this change mean that we've never running linters/formatters on the entire codebase, since the "all files" steps are gone? If we're not, is that safe? I assume it is, since tsc still runs on everything, but wondering if there are any cases where changing a file might cause a problem in an unchanged file and it won't be linted

Yes, it's fine, tsc still runs on everything. The pre-commit action'll commit autofixes to branches, and it's required to merge, so I don't expect tsc to fail on master.

joshuarli avatar Jul 08 '24 18:07 joshuarli

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

github-actions[bot] avatar Jul 08 '24 18:07 github-actions[bot]