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

feat: enhance project configuration with modern setup #5

Merged
merged 1 commit into from
Nov 17, 2024
Merged

Conversation

cheton
Copy link
Collaborator

@cheton cheton commented Nov 17, 2024

PR Type

Enhancement, Tests


Description

  • Refactored Toolpath.js to enhance G-code command handling and modularity.
  • Added a comprehensive test suite for Toolpath covering various G-code scenarios.
  • Introduced ESLint and Babel configurations for improved code quality and modern JavaScript support.
  • Updated package.json to version 3.0.0, revising scripts and dependencies.
  • Added Rollup configuration for module bundling, supporting CommonJS and ESM formats.
  • Implemented GitHub Actions CI workflow for continuous integration.
  • Updated license information and added .eslintignore and .npmignore files.

Changes walkthrough 📝

Relevant files
Enhancement
2 files
Toolpath.js
Refactor and enhance G-code command handling in Toolpath 

src/Toolpath.js

  • Added new handlers for G-code commands.
  • Refactored the Toolpath class for better modularity.
  • Improved position and modal state management.
  • Enhanced coordinate translation functions.
  • +706/-704
    index.js
    Update module export to ES6 default export                             

    src/index.js

    • Changed module export to ES6 default export.
    +1/-1     
    Tests
    3 files
    index.test.js
    Add comprehensive test suite for Toolpath functionality   

    src/tests/index.test.js

  • Added comprehensive test cases for Toolpath.
  • Tested various G-code command scenarios.
  • Verified position and modal state changes.
  • +863/-0 
    t2laser.nc
    Add G-code fixture for T2Laser                                                     

    src/tests/fixtures/t2laser.nc

    • Added G-code fixture for T2Laser.
    +54/-1   
    g92offset.nc
    Add G-code fixture for G92 temporary offsets                         

    src/tests/fixtures/g92offset.nc

    • Added G-code fixture for G92 temporary offsets.
    +32/-1   
    Configuration changes
    7 files
    .eslintrc.js
    Add ESLint configuration for code quality                               

    .eslintrc.js

  • Added ESLint configuration file.
  • Configured environment and parser settings.
  • +14/-0   
    babel.config.js
    Add Babel configuration for modern JavaScript support       

    babel.config.js

  • Added Babel configuration file.
  • Configured presets for modern JavaScript.
  • +6/-0     
    package.json
    Update package configuration for version 3.0.0                     

    package.json

  • Updated project version to 3.0.0.
  • Revised scripts and dependencies for modern setup.
  • Added Jest for testing and Rollup for bundling.
  • +43/-32 
    rollup.config.mjs
    Add Rollup configuration for module bundling                         

    rollup.config.mjs

  • Added Rollup configuration for module bundling.
  • Configured output for CommonJS and ESM formats.
  • +45/-0   
    ci.yml
    Add GitHub Actions CI workflow                                                     

    .github/workflows/ci.yml

  • Added GitHub Actions workflow for CI.
  • Configured Node.js environment and testing steps.
  • +32/-0   
    .npmignore
    Update npmignore for package distribution                               

    .npmignore

    • Added files to ignore in npm package.
    +2/-1     
    .eslintignore
    Add ESLint ignore file                                                                     

    .eslintignore

    • Added directories to ignore for ESLint.
    +3/-0     
    Documentation
    1 files
    LICENSE
    Update license information                                                             

    LICENSE

    • Updated copyright year and holder information.
    +2/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Smell
    The translateZ method in G92 handler uses translateX instead of translateZ for Z coordinate translation, which could lead to incorrect Z-axis positioning

    Error Handling
    Several G-code handlers lack proper error handling and validation of input parameters, which could lead to runtime errors with invalid G-code

    Performance Issue
    Multiple object spread operations and array destructuring in hot code paths could impact performance with large G-code files

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Fix incorrect coordinate translation function used for Z-axis positioning

    In the G92 handler, the Z coordinate translation uses translateX() instead of
    translateZ(). This could cause incorrect Z-axis positioning.

    src/Toolpath.js [476-480]

     if (params.Z !== undefined) {
    -  const zmm = this.translateX(params.Z, false);
    +  const zmm = this.translateZ(params.Z, false);
       this.g92offset.z += this.position.z - zmm;
       this.position.z = zmm;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Critical bug fix - using translateX() instead of translateZ() for Z-axis coordinates would cause incorrect machine positioning, potentially leading to crashes or damaged workpieces.

    9
    Fix incorrect arc direction calculation in counter-clockwise movement

    In the G3 (counter-clockwise arc) handler, the height calculation uses the clockwise
    logic which will result in incorrect arc direction.

    src/Toolpath.js [289-293]

    -if (isClockwise) {
    +if (!isClockwise) {
       height = -height;
     }
     if (radius < 0) {
       height = -height;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Important bug fix - the incorrect height calculation logic in G3 handler would cause counter-clockwise arcs to be drawn in the wrong direction, affecting the accuracy of machined parts.

    8
    Best practice
    Strengthen error testing by verifying specific error messages

    Add error message assertions to verify specific error conditions when testing error
    cases.

    src/tests/index.test.js [15-21]

    -it('should call loadFromFile\'s callback.', (done) => {
    +it('should call loadFromFile\'s callback with proper error.', (done) => {
       toolpath.loadFromFile(null, (err, results) => {
         expect(err).not.toBeNull();
    +    expect(err.message).toMatch(/invalid file/i);
         expect(results).toBeUndefined();
         done();
       });
     });
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding specific error message assertions would make the tests more robust and help catch potential changes in error handling behavior.

    8
    Improve test maintainability by using test lifecycle hooks for setup

    Use beforeEach to instantiate the toolpath object once for each test block instead
    of repeating the instantiation.

    src/tests/index.test.js [6-8]

     describe('Pass a null value as the first argument', () => {
    -  const toolpath = new Toolpath();
    +  let toolpath;
    +  beforeEach(() => {
    +    toolpath = new Toolpath();
    +  });
       it('should call loadFromString\'s callback.', (done) => {
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using beforeEach would reduce code duplication and improve test organization, though the current implementation is still functional.

    5
    Enhancement
    Modernize test code by replacing callback-style tests with async/await pattern

    Use async/await syntax instead of callback-style testing for better readability and
    maintainability.

    src/tests/index.test.js [8-14]

    -it('should call loadFromString\'s callback.', (done) => {
    -  toolpath.loadFromString(null, (err, results) => {
    -    expect(err).toBeNull();
    -    expect(results).toHaveLength(0);
    -    done();
    +it('should handle loadFromString correctly', async () => {
    +  const results = await new Promise((resolve) => {
    +    toolpath.loadFromString(null, (err, res) => {
    +      expect(err).toBeNull();
    +      resolve(res);
    +    });
       });
    +  expect(results).toHaveLength(0);
     });
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using async/await pattern would significantly improve code readability and maintainability, making the test code more modern and easier to understand.

    7
    Enable source maps generation in build output for improved debugging capabilities

    Add source maps generation for better debugging experience in development.

    rollup.config.mjs [17-25]

     output: {
       dir: cjsOutputDirectory,
       format: 'cjs',
       interop: 'auto',
       preserveModules: true,
    +  sourcemap: true,
     },
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding source maps would improve the debugging experience in development, making it easier to track down issues in the bundled code.

    6

    💡 Need additional feedback ? start a PR chat

    @cheton cheton merged commit 54f96ce into master Nov 17, 2024
    1 check passed
    @cheton cheton deleted the feat/v3 branch November 17, 2024 14:44
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant