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

add xyz65 to color convert #264

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Conversation

mck
Copy link
Collaborator

@mck mck commented Jun 11, 2024

PR Type

enhancement


Description

  • Added support for xyz65 color space in the color conversion module.
  • Updated the import statements to include Xyz65 from culori.
  • Included xyz65 in the colorSpaces array.
  • Implemented case handling for xyz65 in the color conversion logic to handle its specific channels and labels.

Changes walkthrough 📝

Relevant files
Enhancement
convert.ts
Add support for `xyz65` color space in color conversion   

packages/graph-engine/src/nodes/color/convert.ts

  • Added support for xyz65 color space in color conversion.
  • Imported Xyz65 from culori.
  • Updated colorSpaces array to include xyz65.
  • Added case handling for xyz65 in the color conversion logic.
  • +19/-4   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    changeset-bot bot commented Jun 11, 2024

    ⚠️ No Changeset found

    Latest commit: ffaa838

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    @github-actions github-actions bot added the enhancement New feature or request label Jun 11, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    2

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    None

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Add a validation check to ensure safe type casting

    To avoid potential runtime errors, add a validation check to ensure 'output' can be
    correctly cast to 'Xyz65' before performing operations on it.

    packages/graph-engine/src/nodes/color/convert.ts [179]

    -const col: Xyz65 = output as unknown as Xyz65;
    +if (!(output instanceof Xyz65)) {
    +  throw new TypeError("Output is not an instance of Xyz65");
    +}
    +const col = output as Xyz65;
     
    Suggestion importance[1-10]: 10

    Why: Adding a validation check before type casting is essential for preventing potential runtime errors and ensuring type safety, which is a significant improvement for security and reliability.

    10
    Enhancement
    Add unit tests for the new 'xyz65' color space to ensure correct functionality

    To ensure the new color space 'xyz65' integrates seamlessly, consider adding unit tests
    that cover its conversion logic and edge cases.

    packages/graph-engine/src/nodes/color/convert.ts [177]

    -case "xyz65":
    +// Example unit test for xyz65 color space conversion
    +describe('xyz65 color space conversion', () => {
    +  it('should convert correctly', () => {
    +    const input = { x: 0.5, y: 0.5, z: 0.5, alpha: 1 };
    +    const expectedOutput = { a: 0.5, b: 0.5, c: 0.5, d: 1, channels: [0.5, 0.5, 0.5, 1], labels: ["x", "y", "z", "alpha"] };
    +    expect(convertColor(input)).toEqual(expectedOutput);
    +  });
    +});
     
    Suggestion importance[1-10]: 9

    Why: Adding unit tests for the new 'xyz65' color space is crucial for ensuring its correct functionality and handling edge cases. This enhances the robustness of the code.

    9
    Maintainability
    Improve type assertion for better readability and performance

    Consider using a type assertion instead of casting twice for better readability and
    performance. TypeScript allows direct type assertions which are more straightforward.

    packages/graph-engine/src/nodes/color/convert.ts [179]

    -const col: Xyz65 = output as unknown as Xyz65;
    +const col = output as Xyz65;
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves code readability and performance by using a direct type assertion instead of casting twice. It is a straightforward and beneficial change.

    8
    Best practice
    Use object destructuring for cleaner and more readable assignment

    Consider destructuring the 'col' object directly in the assignment to 'final' to simplify
    the code and improve readability.

    packages/graph-engine/src/nodes/color/convert.ts [179-186]

    -const col: Xyz65 = output as unknown as Xyz65;
    +const { x, y, z, alpha }: Xyz65 = output as unknown as Xyz65;
     final = {
    -  a: col.x,
    -  b: col.y,
    -  c: col.z,
    -  d: col.alpha,
    -  channels: [col.x, col.y, col.z, col.alpha],
    +  a: x,
    +  b: y,
    +  c: z,
    +  d: alpha,
    +  channels: [x, y, z, alpha],
       labels: ["x", "y", "z", "alpha"],
     };
     
    Suggestion importance[1-10]: 7

    Why: Using object destructuring simplifies the code and improves readability. While not critical, it is a good practice that enhances maintainability.

    7

    @roppazvan roppazvan merged commit cb474b0 into major/strong-typing Jun 12, 2024
    2 checks passed
    @SorsOps SorsOps deleted the add-xyz65-to-convert branch July 3, 2024 09:33
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants