Skip to content

Conversation

@opbot-xd
Copy link
Contributor

Description

This PR addresses two related issues to improve the developer experience and CI consistency:

  1. Line Endings Standardization (Fix inconsistent line endings causing CI failures on Windows #729): Resolves CI failures that occur when Windows contributors submit PRs with CRLF line endings. The Ruff config now enforces lf line endings, and a .gitattributes file ensures Git normalizes all text files to LF.

  2. Frontend Linters in Pre-commit (Integrate Frontend Linters into Pre-commit Workflow #727): Integrates Prettier and ESLint into the pre-commit workflow using local npm scripts, matching the exact same commands used in CI. This catches formatting and linting issues before commit rather than at CI time.

Related issues

Type of change

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist

  • I have read and understood the rules about how to Contribute to this project.
  • The pull request is for the branch develop.
  • I have added documentation of the new features.
  • Linter (Ruff) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.
  • I have added tests for the feature/bug I solved. All the tests (new and old ones) gave 0 errors.
  • If changes were made to an existing model/serializer/view, the docs were updated and regenerated (check CONTRIBUTE.md).
  • If the GUI has been modified:
    • I have a provided a screenshot of the result in the PR.
    • I have created new frontend tests for the new component or updated existing ones.

…-commit

Fixes intelowlproject#729: Fix inconsistent line endings causing CI failures on Windows
- Update Ruff config to enforce LF line endings (line-ending = 'lf')
- Add .gitattributes to normalize line endings via Git

Fixes intelowlproject#727: Integrate Frontend Linters into Pre-commit Workflow
- Add Prettier hook using local npm script (npm run formatter)
- Add ESLint hook using local npm script (npm run lint)
- Uses same linter versions as CI to ensure consistency
@opbot-xd
Copy link
Contributor Author

Question for Maintainers: NPM Dependencies Requirement

@mlodic @regulartim
The frontend linter hooks (prettier and eslint) use local npm scripts (npm run formatter and npm run lint) to ensure consistency with CI.

This means developers need to:

  1. Run npm install in the frontend/ directory
  2. Run npm run lint-config-install in the frontend/ directory (to install eslint-config-certego dependencies)

Before pre-commit install will work correctly for frontend files.

Options to consider:

Option A: Document the requirement (current approach)

  • Add a note in CONTRIBUTING.md that developers must run:
    cd frontend && npm install && npm run lint-config-install
    before using pre-commit hooks.

Option B: Skip frontend hooks if npm not installed

  • Modify hooks to gracefully skip if node_modules doesn't exist
  • Less strict but avoids setup friction

Option C: Use pre-commit mirrors (rejected)

  • We initially tried pre-commit/mirrors-prettier and pre-commit/mirrors-eslint
  • This caused issues because the mirror versions differ from project versions (Prettier 3.1.0 vs 2.8.4), causing formatting conflicts with CI
  • Also had duplicate plugin conflicts with eslint-config-react-app

@regulartim
Copy link
Collaborator

I don't have a strong opinion on this but tend to option A. I would like to hear what @mlodic thinks about this.

@mlodic
Copy link
Member

mlodic commented Jan 23, 2026

I wouldn't enforce the requirement because some people could never work on the frontend. I vote B

@regulartim
Copy link
Collaborator

I wouldn't enforce the requirement because some people could never work on the frontend. I vote B

Yeah, good point, I agree.

@opbot-xd
Copy link
Contributor Author

I agree with Option B as well.

A one-time CI linting failure is much more acceptable than having contributors inadvertently modify package-lock.json due to a forced npm install during pre-commit. This keeps the pre-commit lightweight for backend-only contributors while still catching frontend issues in CI.

I'll update the hooks to gracefully skip if node_modules doesn't exist.

Frontend pre-commit hooks (prettier, eslint) now gracefully skip if
frontend/node_modules doesn't exist. This allows backend-only contributors
to use pre-commit without needing to run npm install in frontend/.
@opbot-xd
Copy link
Contributor Author

The pre-commit is wokring fine.
image

image

@opbot-xd
Copy link
Contributor Author

@regulartim

I've implemented Option B and the PR is ready for review. I've attached a screenshot showing the pre-commit hooks working correctly (including the skip behavior when node_modules is not installed).

However, I'm having difficulty figuring out how to properly test the line-ending changes locally:

  1. .github/configurations/python_linters/.ruff.toml - Changed line-ending = "native"line-ending = "lf"
  2. .gitattributes (new file) - Added to normalize all text files to LF

Could you advise on the best way to verify these changes work correctly for Windows contributors? The main goal is to ensure CRLF line endings from Windows don't cause CI failures anymore.

Thank you!

@opbot-xd opbot-xd marked this pull request as ready for review January 23, 2026 16:34
Copilot AI review requested due to automatic review settings January 23, 2026 16:34
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes line endings to LF across all platforms and integrates frontend linters into the pre-commit workflow to catch formatting issues before CI.

Changes:

  • Updated Ruff configuration to enforce LF line endings instead of platform-native endings
  • Added comprehensive .gitattributes file to normalize all text files to LF on checkout and commit
  • Integrated Prettier and ESLint into pre-commit using local npm scripts with graceful fallback for backend-only contributors

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
.github/configurations/python_linters/.ruff.toml Changed line-ending setting from "native" to "lf" to enforce consistent Unix-style line endings
.github/.pre-commit-config.yaml Added Prettier and ESLint hooks using local npm scripts that match CI commands, with graceful degradation for contributors without node_modules
.gitattributes Added comprehensive Git attributes configuration to normalize line endings for all text files and mark binary files appropriately

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@regulartim
Copy link
Collaborator

Nice, thank you @opbot-xd ! I guess you could test by taking a python file from the code base, change the line endings to CRLF (most editors can do that) and try to commit it. Of course it would also be nice if a windows user would test it. @shivraj1182 , would you do that?

@opbot-xd
Copy link
Contributor Author

Tested the Line Endings Fix

Thanks for the suggestion @regulartim! I tested by converting a Python file to CRLF line endings:

Test performed:

  1. Converted api/urls.py to CRLF: sed -i 's/$/\r/' api/urls.py
  2. Verified CRLF: file api/urls.py"with CRLF line terminators"
  3. Ran pre-commit: pre-commit run ruff-format --files api/urls.py
  4. Result: Ruff automatically reformatted the file → "1 file reformatted"
  5. Verified LF: file api/urls.py"Python script, ASCII text executable" (no more CRLF)

The line-ending = "lf" setting in Ruff config is working correctly - CRLF files are automatically converted to LF during pre-commit.

@shivraj1182
Copy link

@opbot-xd @regulartim, This looks good to me, thanks for handling the Windows line-ending issues and frontend linters!

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

Successfully merging this pull request may close these issues.

4 participants