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

refactor(xo): removed ignores #3761

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

mfranzke
Copy link
Collaborator

@mfranzke mfranzke commented Feb 1, 2025

Proposed changes

we shouldn't ignore xo in general. xo is here to hurt our feelings. If it does, that's a good thing.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (fix on existing components or architectural decisions)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Further comments

we shouldn't ignore `xo` in general. `xo` is here to hurt our feelings. If it does, that's a good thing.
@mfranzke mfranzke added the 🍄🆙improvement New feature or request label Feb 1, 2025
@mfranzke mfranzke self-assigned this Feb 1, 2025
@mfranzke mfranzke marked this pull request as draft February 1, 2025 13:45
Copy link
Contributor

github-actions bot commented Feb 1, 2025

🔭🐙🐈 Test this branch here: https://db-ui.github.io/mono/review/refactor-removed-xo-ignores

@github-actions github-actions bot added the 🏗foundations Changes inside foundations folder label Feb 1, 2025
@mfranzke mfranzke requested review from Copilot and removed request for bruno-sch March 28, 2025 14:37
Copy link

@Copilot 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 refactors the migration and configuration files by removing unnecessary ignore patterns and updating import statements and file headers. Key changes include:

  • Removing directories from .xo-config ignores to enforce linting on migration and foundations code.
  • Updating type assertions and file path imports to include proper extensions and shebang directives.
  • Standardizing import usage (e.g., using node built-ins) and simplifying error handling in the fonts generation script.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/migration/test//.spec.ts Replaced explicit type cast with non-null assertion in test expectations.
packages/migration/src/types.ts Updated shebang and import paths/extensions along with type definition.
packages/migration/src/program.ts Added a shebang directive for immediate execution.
packages/migration/src/migration/index.ts Standardized import formatting and added a shebang revision.
packages/migration/src/data.ts Revised import paths to include file extensions.
packages/foundations/assets/fonts/generate-eu-fonts.ts Replaced legacy path imports with node built-ins and simplified error catch.
.xo-config.cjs Removed ignored patterns to ensure linting is applied uniformly.
Comments suppressed due to low confidence (1)

packages/migration/test/v0.0.6-v0.0.7/v006_v007.spec.ts:15

  • The non-null assertion operator (!) assumes result is always defined, which may mask potential issues. Consider validating result explicitly to ensure stability in unexpected conditions.
expect(result!.filter((res) => res.hasChanged)).toHaveLength(1);

Comment on lines +16 to 18
} catch {
console.warn(
'You need to install pyftsubset. Check packages/foundations/assets/fonts/README.md for more information.'
Copy link
Preview

Copilot AI Mar 28, 2025

Choose a reason for hiding this comment

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

By omitting the error parameter in the catch block, important error details might be lost during debugging. Consider capturing the error (e.g., 'catch (e)') and logging its message for improved traceability.

Suggested change
} catch {
console.warn(
'You need to install pyftsubset. Check packages/foundations/assets/fonts/README.md for more information.'
} catch (e) {
console.warn(
`You need to install pyftsubset. Check packages/foundations/assets/fonts/README.md for more information. Error: ${e.message}`

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏗foundations Changes inside foundations folder 🍄🆙improvement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant