Cleanup pre-commit setup (streamlit#9856)

## Describe your changes This PR applies a couple of improvements & cleanups related to pre-commit: - Remove pre commit install from shared `make_init` action. - Move enforcement of pre-commit rules from `js-tests` to dedicated workflow (takes < 1 min) -> missing license headers in python files will not cause the js-tests workflow to fail anymore. - Use make commands in js-tests workflow - Remove eslint, typecheck-app, typecheck-lib from pre-commit since these checks are not actually run as part of the normal pre-commit. - Update versions in pre-commit config. - Add some additional pre-commit checks: check-json, check-yaml, check-toml, check-added-large-files. - Fix some json / yml issues. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
BrainGithub · Nov 13, 2024 · a5e3afb · a5e3afb
1 parent 048aaa5
commit a5e3afb
Show file tree

Hide file tree

Showing 10 changed files with 81 additions and 63 deletions.
diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml
@@ -1,6 +1,6 @@
 name: ✨ Feature request
 description: Suggest a feature or enhancement for Streamlit
-labels: [type:enhancement]
+labels: ["type:enhancement"]
 body:
   - type: markdown
     attributes:

diff --git a/.github/actions/make_init/action.yml b/.github/actions/make_init/action.yml
@@ -11,17 +11,6 @@ inputs:
 runs:
   using: composite
   steps:
-    - name: Restore pre-commit cache
-      id: cache-pre-commit
-      uses: actions/cache@v4
-      with:
-        path: ~/.cache/pre-commit
-        key: v1-pre-commit-${{ env.pythonLocation }}-${{ hashFiles('**/.pre-commit-config.yaml') }}
-    - name: Install pre-commit
-      run: |
-        pip install pre-commit
-        pre-commit install-hooks
-      shell: bash
     - name: Setup Node
       uses: actions/setup-node@v4
       with:

diff --git a/.github/workflows/enforce-pre-commit.yml b/.github/workflows/enforce-pre-commit.yml
@@ -0,0 +1,57 @@
+name: Enforce Pre-Commit Hooks
+
+on:
+  push:
+    branches:
+      - "develop"
+  pull_request:
+    types: [opened, synchronize, reopened]
+  # Allows workflow to be called from other workflows
+  workflow_call:
+    inputs:
+      ref:
+        required: true
+        type: string
+
+# Avoid duplicate workflows on same branch
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}-enforce-pre-commit
+  cancel-in-progress: true
+
+jobs:
+  enforce-pre-commit:
+    runs-on: ubuntu-latest
+
+    defaults:
+      run:
+        shell: bash
+
+    steps:
+      - name: Checkout Streamlit code
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ inputs.ref }}
+          persist-credentials: false
+          submodules: "recursive"
+          fetch-depth: 2
+      - name: Set Python version vars
+        uses: ./.github/actions/build_info
+      - name: Set up Python ${{ env.PYTHON_MAX_VERSION }}
+        uses: actions/setup-python@v5
+        with:
+          python-version: "${{ env.PYTHON_MAX_VERSION }}"
+      - name: Restore pre-commit cache
+        id: cache-pre-commit
+        uses: actions/cache@v4
+        with:
+          path: ~/.cache/pre-commit
+          key: v1-pre-commit-${{ env.pythonLocation }}-${{ hashFiles('**/.pre-commit-config.yaml') }}
+      - name: Install pre-commit
+        run: |
+          pip install pre-commit
+          pre-commit install-hooks
+        shell: bash
+      - name: Install prettier
+        run: cd frontend && yarn install
+      - name: Run pre-commit hooks
+        run: PRE_COMMIT_NO_CONCURRENCY=true pre-commit run --show-diff-on-failure --color=always --all-files
diff --git a/.github/workflows/ensure-relative-imports.yml b/.github/workflows/ensure-relative-imports.yml
@@ -48,5 +48,5 @@ jobs:
         run: make protobuf
       - name: Run make frontend-lib-prod
         run: make frontend-lib-prod
-      - name: Ensure ensure relative imports exist in the @streamlit/lib/dist folder
+      - name: Ensure relative imports exist in the @streamlit/lib/dist folder
         run: make ensure-relative-imports
diff --git a/.github/workflows/js-tests.yml b/.github/workflows/js-tests.yml
@@ -49,13 +49,10 @@ jobs:
         run: make frontend-lib
       - name: Audit frontend licenses
         run: ./scripts/audit_frontend_licenses.py
+      - name: Run type checks
+        run: make tstypecheck
       - name: Run linters
-        run: |
-          # Run eslint as a standalone command to generate the test report.
-          # We need to --hook-stage manual to trigger Typecheck too
-          PRE_COMMIT_NO_CONCURRENCY=true SKIP=eslint,ruff,ruff-format pre-commit run --hook-stage manual --show-diff-on-failure --color=always --all-files
-          # Run eslint using Makefile omitting the pre-commit
-          make jslint
+        run: make jslint
       - name: Validate NOTICES
         run: |
           # Run `make notices`. If it results in changes, warn the user and fail.

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -53,48 +53,14 @@ repos:
         files: ^.github/.*\.(yml|yaml)$
         language: node
         pass_filenames: false
-      # Typecheck will be run on CI using --hook-stage manual flag,
-      # check out .github/workflows/js-tests.yml "Run linters" step for details.
-      - id: typecheck-app
-        name: Typecheck App
-        always_run: true
-        entry: yarn --cwd frontend/app tsc --noEmit
-        files: \.(js|jsx|ts|tsx)$
-        language: node
-        pass_filenames: false
-        stages:
-          - manual
-      - id: typecheck-lib
-        name: Typecheck Lib
-        always_run: true
-        # no noEmit flag bc tsconfig.json emitDeclaration key is used
-        entry: yarn --cwd frontend/lib tsc
-        files: \.(js|jsx|ts|tsx)$
-        language: node
-        pass_filenames: false
-        stages:
-          - manual
-      # Eslint will be run on CI using Makefile,
-      # check out .github/workflows/js-tests.yml "Run linters" step for details.
-      - id: eslint
-        name: Eslint
-        always_run: true
-        # TODO: This hook currently does not work on Windows due to "yarn" not being an executable and win32api.CreateProcess
-        # turning `subprocess.run(["yarn", "prettier", "--write"])` into a call to `yarn.exe prettier --write` which does not exist
-        entry: ./scripts/run_in_subdirectory.py frontend/ yarn lint --fix
-        files: ^frontend/src/.*\.(js|jsx|ts|tsx)$
-        language: node
-        pass_filenames: true
-        stages:
-          - manual
       - id: license-headers
         name: Checks that all files have the required license headers
         entry: ./scripts/check_license_headers.py
         language: system
         always_run: true
         pass_filenames: false
   - repo: https://github.com/Lucas-C/pre-commit-hooks
-    rev: v1.5.4
+    rev: v1.5.5
     hooks:
       - id: insert-license
         name: Add license for all (S)CSS/JS(X)/TS(X) files
@@ -171,7 +137,7 @@ repos:
           |^vendor/
           |^component-lib/declarations/apache-arrow
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v4.3.0
+    rev: v5.0.0
     hooks:
       - id: trailing-whitespace
         exclude: |
@@ -180,3 +146,9 @@ repos:
           |^NOTICES$
           |^proto/streamlit/proto/openmetrics_data_model.proto$
           |\.snap$
+      - id: check-added-large-files
+      - id: check-json
+        exclude: .vscode/launch.json
+      - id: check-toml
+      - id: check-yaml
+        exclude: lib/conda-recipe/meta.yaml
diff --git a/Makefile b/Makefile
@@ -302,20 +302,20 @@ frontend-app:
 	cd frontend/ ; yarn run buildApp
 
 .PHONY: jslint
-# Lint the JS code.
+# Verify that our JS/TS code is formatted and that there are no lint errors.
 jslint:
-	cd frontend; \
-		yarn lint;
+	cd frontend/ ; yarn run formatCheck
+	cd frontend/ ; yarn run lint
 
 .PHONY: tstypecheck
 # Typecheck the JS/TS code.
 tstypecheck:
-	pre-commit run typecheck-lib --all-files --hook-stage manual && pre-commit run typecheck-app --all-files --hook-stage manual
+	cd frontend/ ; yarn run typecheck
 
 .PHONY: jsformat
 # Fix formatting issues in our JavaScript & TypeScript files.
 jsformat:
-	pre-commit run prettier --all-files --hook-stage manual
+	cd frontend/ ; yarn run format
 
 .PHONY: jstest
 # Run JS unit tests.

diff --git a/frontend/.prettierignore b/frontend/.prettierignore
@@ -1 +1,3 @@
 vendor
+lib/src/proto.d.ts
+lib/src/proto.js
diff --git a/frontend/lib/tsconfig.prod.json b/frontend/lib/tsconfig.prod.json
@@ -2,5 +2,5 @@
     "extends": "./tsconfig.json",
     "compilerOptions": {
       "paths": { "@streamlit/lib/src/*": ["src/*"] }
-    },
-  }
+  }
+}
diff --git a/frontend/package.json b/frontend/package.json
@@ -18,7 +18,8 @@
     "lint": "yarn lintApp && yarn lintLib",
     "lintApp": "yarn workspace @streamlit/app lint",
     "lintLib": "yarn workspace @streamlit/lib lint",
-    "prettier": "prettier --write --config .prettierrc app/src && prettier --write --config .prettierrc lib/src",
+    "format": "prettier --write --config .prettierrc './{app,lib}/src/**/*.{js,ts,jsx,tsx}'",
+    "formatCheck": "prettier --check --config .prettierrc './{app,lib}/src/**/*.{js,ts,jsx,tsx}'",
     "test": "yarn workspace @streamlit/lib test && yarn workspace @streamlit/app test",
     "testcoverage": "yarn workspace @streamlit/lib test --coverage && yarn workspace @streamlit/app test --coverage",
     "testLib": "yarn workspace @streamlit/lib testWatch",