perf: code quality: enable Biome linter, expand ruff rules, fix lint issues#82
Conversation
…t issues Frontend: - Enable Biome linter with recommended rules (was disabled) - Configure jsxRuntime as reactClassic for proper React import handling - Auto-fix: type-only imports, == to ===, template literals, optional chaining, unused imports, let-to-const, literal keys, date constructors - Manual fixes: remove unused variables, add alt text, button types, proper TypeScript types replacing `any`, remove empty destructuring patterns, replace `<></>` with null, fix empty type aliases - Set error level for correctness rules, warn level for style/a11y for gradual cleanup Backend: - Add PERF, PLE, PLW, PGH, FLY, FURB, PIE rule sets to ruff config - Auto-fix: unused noqa directives, import sorting - Fix explicit return in __init__ methods (PLE0101) - Add noqa for intentional wildcard import in local_settings - Suppress RUF012 (mutable class variables) - too noisy for Django models - Add migration-specific ignores for T20, PERF, ARG001 Infrastructure: - Add biome lint step to `just lint-frontend` recipe https://claude.ai/code/session_01V3hbn1ExGmpbJZrzF6zCs3
There was a problem hiding this comment.
Code Review
This pull request updates project dependencies, including a version bump for olmap and olmap_ui, and updates several Sentry packages. It also enhances the linting configuration by adding new rules to ruff and enabling biome for the frontend. Additionally, it cleans up various unused arguments in Python methods and improves TypeScript type imports. I have identified several critical issues where removing import React from 'react' breaks the build due to the reactClassic JSX runtime configuration, and I have provided feedback on improving unused argument handling.
| @@ -1,4 +1,3 @@ | |||
| import React from 'react'; | |||
| import { render } from '@testing-library/react'; | |||
There was a problem hiding this comment.
The jsxRuntime is set to reactClassic in biome.json, which requires React to be in scope when using JSX. Since this file uses JSX (<App />), removing the import React from 'react'; statement will cause a 'React is not defined' error. This import should be restored.
| import { render } from '@testing-library/react'; | |
| import React from 'react'; | |
| import { render } from '@testing-library/react'; |
| @@ -1,5 +1,3 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const UpdateWay = ({ changesetId, way }) => ( | |||
There was a problem hiding this comment.
The jsxRuntime is set to reactClassic in biome.json, which requires React to be in scope when using JSX. Removing the import React from 'react'; statement will cause a 'React is not defined' error. This import should be restored.
import React from 'react';
const UpdateWay = ({ changesetId, way }) => (| @@ -1,5 +1,3 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const UpdateNode = ({ changesetId, node, tags }) => ( | |||
There was a problem hiding this comment.
The jsxRuntime is set to reactClassic in biome.json, which requires React to be in scope when using JSX. Removing the import React from 'react'; statement will cause a 'React is not defined' error. This import should be restored.
import React from 'react';
const UpdateNode = ({ changesetId, node, tags }) => (| @@ -1,5 +1,3 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const CreateWay = ({ changesetId, way }) => ( | |||
There was a problem hiding this comment.
The jsxRuntime is set to reactClassic in biome.json, which requires React to be in scope when using JSX. Removing the import React from 'react'; statement will cause a 'React is not defined' error. This import should be restored.
import React from 'react';
const CreateWay = ({ changesetId, way }) => (| @@ -1,5 +1,3 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const CreateNode = ({ changesetId, lon, lat, tags }) => ( | |||
There was a problem hiding this comment.
The jsxRuntime is set to reactClassic in biome.json, which requires React to be in scope when using JSX. Removing the import React from 'react'; statement will cause a 'React is not defined' error. This import should be restored.
import React from 'react';
const CreateNode = ({ changesetId, lon, lat, tags }) => (| @with_parameters(["name"]) | ||
| @action(methods=["GET"], detail=False) | ||
| def search(self, request, *args, **kwargs): # noqa: ARG002 | ||
| def search(self, request, *args, **kwargs): |
| field_class = serializers.PrimaryKeyRelatedField | ||
|
|
||
| def convert(self, field): # noqa: ARG002 | ||
| def convert(self, field): |
There was a problem hiding this comment.
The field argument is unused in this method. To signal that it's intentionally unused and to satisfy the ARG002 linting rule, you should prefix it with an underscore. This is a common Python convention and is cleaner than using a noqa comment.
This feedback applies to the convert methods in StringRelatedFieldConverter, HyperlinkedRelatedFieldConverter, and SlugRelatedFieldConverter as well.
| def convert(self, field): | |
| def convert(self, _field): |
| from .map_feature_tests import * | ||
| from .osm_image_notes_tests import * | ||
| from .rest_tests import * |
There was a problem hiding this comment.
Wildcard imports (from ... import *) are generally discouraged as they make it unclear which names are present in the namespace. This can lead to confusion and make the code harder to read and maintain. It seems the linter configuration now flags this (F403).
If these wildcard imports are necessary for the test suite setup, you should add a # noqa: F403 comment to each line to explicitly suppress the linting error. However, a better long-term solution would be to explicitly import the required names from each test module.
| from .map_feature_tests import * | |
| from .osm_image_notes_tests import * | |
| from .rest_tests import * | |
| from .map_feature_tests import * # noqa: F403 | |
| from .osm_image_notes_tests import * # noqa: F403 | |
| from .rest_tests import * # noqa: F403 |
django_server/olmap/health.py
Outdated
|
|
||
|
|
||
| def ready_check(request): # noqa: ARG001 | ||
| def ready_check(request): |
django_server/olmap/health.py
Outdated
|
|
||
|
|
||
| def health_check(request): # noqa: ARG001 | ||
| def health_check(request): |
|
Let's address the recommendations in the comments. Evaluate which pieces of criticism and recommendations are substantive and valid. Act accordingly and refute invalid points. |
|
Claude finished @laurigates's task in 3m 31s —— View job Review Comment Evaluation & Fixes
Invalid Comments (Refuted)"Critical": React import removals in The reviewer's concern is incorrect. This project uses React 18 + react-scripts 5.0.1 (CRA), which uses Babel with the automatic JSX transform — Medium: Remove DRF's dispatch calls action handlers as Valid Fixes ImplementedCommit
|
- Rename request -> _request in health_check and ready_check (ARG001) - Rename field -> _field in StringRelatedField, HyperlinkedRelatedField, SlugRelatedField converters (ARG002) - Remove spurious pk=None from map_feature_schemas (detail=False action) Co-authored-by: Lauri Gates <laurigates@users.noreply.github.com>
Frontend:
unused imports, let-to-const, literal keys, date constructors
proper TypeScript types replacing
any, remove empty destructuring patterns,replace
<></>with null, fix empty type aliasesBackend:
Infrastructure:
just lint-frontendrecipehttps://claude.ai/code/session_01V3hbn1ExGmpbJZrzF6zCs3