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

fest: support parsing nested comments #9

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

cheton
Copy link
Collaborator

@cheton cheton commented Nov 20, 2024

PR Type

Enhancement, Tests


Description

  • Enhanced the comment parsing logic to support nested comments and multiple comments in a line.
  • Introduced new line modes ('original', 'stripped', 'compact') for parsing, allowing more flexible handling of comments and whitespace.
  • Added comprehensive tests to verify the correct parsing of nested comments and multiple comments in a line.
  • Refactored code to improve clarity and maintainability, including renaming variables and updating test descriptions.

Changes walkthrough 📝

Relevant files
Tests
index.test.js
Add tests for nested and multiple comments parsing             

src/tests/index.test.js

  • Added tests for parsing nested comments and multiple comments in a
    line.
  • Renamed variables for clarity.
  • Updated test descriptions for better understanding.
  • +65/-47 
    Enhancement
    index.js
    Enhance comment parsing with nested support and line modes

    src/index.js

  • Introduced new line modes: 'original', 'stripped', and 'compact'.
  • Refactored comment stripping logic to handle nested comments.
  • Added default options for parsing functions.
  • Improved handling of options in parsing functions.
  • +79/-48 

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

    Copy link

    codiumai-pr-agent-free bot commented Nov 20, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 82c8b26)

    Here are some key observations to aid the review process:

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

    Possible Bug
    The nested parentheses parsing logic may have edge cases around escaped characters or malformed nested comments that could cause incorrect parsing

    Code Smell
    The _stripComments function is quite complex with multiple nested conditions and state tracking. Consider breaking it down into smaller functions for better maintainability

    Performance Issue
    The comment parsing now involves character-by-character processing which could be slower for large files compared to the previous regex-based approach

    Copy link

    codecov bot commented Nov 20, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 93.46%. Comparing base (b3bdf84) to head (fcceaba).
    Report is 1 commits behind head on master.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##           master       #9      +/-   ##
    ==========================================
    + Coverage   92.69%   93.46%   +0.77%     
    ==========================================
      Files           1        1              
      Lines         178      199      +21     
    ==========================================
    + Hits          165      186      +21     
      Misses         13       13              

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.


    🚨 Try these New Features:

    @cheton cheton force-pushed the feat/nested-comments branch from 50e89aa to 82c8b26 Compare November 20, 2024 12:28
    @cheton
    Copy link
    Collaborator Author

    cheton commented Nov 20, 2024

    /review

    @cheton
    Copy link
    Collaborator Author

    cheton commented Nov 20, 2024

    /improve

    Copy link

    Persistent review updated to latest commit 82c8b26

    src/index.js Outdated Show resolved Hide resolved
    @cheton cheton force-pushed the feat/nested-comments branch from 899f06a to 3e81545 Compare November 20, 2024 12:34
    @cncjs cncjs deleted a comment from codiumai-pr-agent-free bot Nov 20, 2024
    @cheton
    Copy link
    Collaborator Author

    cheton commented Nov 20, 2024

    /improve

    @cncjs cncjs deleted a comment from codiumai-pr-agent-free bot Nov 20, 2024
    @cncjs cncjs deleted a comment from codiumai-pr-agent-free bot Nov 20, 2024
    @cheton
    Copy link
    Collaborator Author

    cheton commented Nov 20, 2024

    /improve

    Copy link

    codiumai-pr-agent-free bot commented Nov 20, 2024

    PR Code Suggestions ✨

    Latest suggestions up to fcceaba

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for unclosed parentheses in comment parsing to handle malformed input gracefully

    Add a check for unclosed parentheses in the comment parsing logic to prevent
    potential issues with malformed input.

    src/index.js [85-89]

     const _stripComments = (line) => {
       let result = '';
       let currentComment = '';
       let comments = [];
       let openParens = 0;
    +  
    +  // Track initial position of opening parenthesis for error reporting
    +  let initialParenPos = -1;
     
    +  for (let i = 0; i < line.length; i++) {
    +    if (line[i] === '(' && openParens === 0) {
    +      initialParenPos = i;
    +    }
    +    // ... rest of the code ...
    +  }
    +  
    +  if (openParens > 0 && initialParenPos !== -1) {
    +    console.warn(`Warning: Unclosed parenthesis at position ${initialParenPos} in line: ${line}`);
    +  }
    +
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential robustness issue by adding validation for unclosed parentheses, which could help detect and handle malformed G-code input. This is important for maintaining code reliability.

    8
    Add input validation to prevent runtime errors from invalid parameters

    Handle potential undefined options object in parseLine to prevent runtime errors.

    src/index.js [147-152]

     return (line, options = {}) => {
    +  if (typeof line !== 'string') {
    +    throw new Error('Line must be a string');
    +  }
       const flatten = !!(options?.flatten ?? DEFAULT_FLATTEN);
       let lineMode = options?.lineMode ?? DEFAULT_LINE_MODE;
       if (!LINE_MODES.includes(options?.lineMode)) {
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion enhances code robustness by adding crucial input validation for the line parameter, preventing potential runtime errors from invalid input types.

    8
    Use a reasonable maximum limit for nested parentheses to prevent integer overflow

    Prevent potential integer overflow in the openParens counter by using a more
    reasonable maximum value instead of Number.MAX_SAFE_INTEGER.

    src/index.js [109]

    -openParens = Math.min(openParens + 1, Number.MAX_SAFE_INTEGER);
    +openParens = Math.min(openParens + 1, 1000); // Limit nested parentheses to prevent overflow
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code safety by preventing potential integer overflow issues with deeply nested parentheses, using a more practical limit instead of MAX_SAFE_INTEGER.

    7

    Previous suggestions

    ✅ Suggestions up to commit 3e81545
    CategorySuggestion                                                                                                                                    Score
    Security
    ✅ Prevent integer overflow by adding upper bounds to numeric counters

    The openParens counter should have an upper limit to prevent potential integer
    overflow in case of malformed input with too many opening parentheses.

    src/index.js [101-109]

     if (char === '(') {
       // Start parentheses comment
       if (openParens === 0) {
         currentComment = '';
       } else if (openParens > 0) {
         currentComment += char;
       }
    -  openParens += 1;
    +  openParens = Math.min(openParens + 1, Number.MAX_SAFE_INTEGER);
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    Why: Adding an upper bound to the openParens counter is a good security practice to prevent potential integer overflow attacks from malformed input, which could lead to parsing errors or unexpected behavior.

    7
    Possible issue
    ✅ Reset state variables when encountering a comment terminator to prevent parsing issues with subsequent lines

    The openParens counter should be reset when a semicolon comment is encountered
    outside parentheses to prevent potential parsing issues with unclosed parentheses.

    src/index.js [95-99]

     if (char === ';' && openParens === 0) {
       // Start semicolon comment outside parentheses
       comments.push(line.slice(i + 1).trim());
    +  openParens = 0; // Reset parentheses counter
       break; // Stop further processing after a semicolon comment
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 4

    Why: While resetting openParens after a semicolon comment is a good practice for state management, it's not critical since the break statement already ensures no further processing occurs, and the variable is reset for each new line anyway.

    4
    ✅ Suggestions up to commit 82c8b26
    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Prevent potential parsing errors by handling unmatched parentheses in comment parsing

    Add validation for openParens to prevent negative values in case of unmatched
    parentheses. If openParens becomes negative, it indicates malformed input that could
    lead to incorrect parsing.

    src/index.js [109-118]

     } else if (char === ')') {
       // End parentheses comment
    -  openParens -= 1;
    +  openParens = Math.max(0, openParens - 1);  // Prevent negative values
       if (openParens === 0) {
         comments.push(currentComment.trim());
         currentComment = '';
       } else if (openParens > 0) {
         currentComment += char;
       }
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential bug where unmatched parentheses could lead to negative openParens values, causing incorrect parsing results. This is a significant improvement for code robustness and error handling.

    8
    General
    ✅ Add error validation in async callback to ensure operation success

    The test case is missing an error check in the callback. Add error validation to
    ensure the parsing operation completed successfully.

    src/tests/index.test.js [39-42]

     parseString(inputText, (err, results) => {
    +  expect(err).toBeNull();
       expect(results.length).toBe(0);
       done();
     });

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    Why: Adding error validation in the test callback is important for ensuring proper error handling and test reliability. Missing error checks could hide potential issues in the parsing operation.

    7
    Suggestions up to commit 50e89aa
    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for malformed input with unmatched parentheses to prevent incorrect parsing

    Add validation for unclosed parentheses in the _stripComments function. If
    openParens is greater than 0 at the end of processing, it means there are unclosed
    parentheses which could lead to incorrect comment parsing.

    src/index.js [81-85]

     const _stripComments = (line) => {
       let result = '';
       let currentComment = '';
       let comments = [];
       let openParens = 0;
    +  
    +  if (line.split('(').length !== line.split(')').length) {
    +    throw new Error('Unmatched parentheses in comment');
    +  }
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical edge case where unmatched parentheses could lead to incorrect parsing of G-code comments, potentially causing runtime errors or silent failures. This validation would improve code robustness significantly.

    8
    Add input validation to prevent runtime errors from invalid input types

    The _stripComments function should handle potential undefined or null input to
    prevent runtime errors. Add input validation at the start of the function.

    src/index.js [81-84]

     const _stripComments = (line) => {
    +  if (line == null) return ['', []];
    +  line = String(line);
       let result = '';
       let currentComment = '';
       let comments = [];
    Suggestion importance[1-10]: 7

    Why: This suggestion addresses an important defensive programming practice by adding input validation, which would prevent potential runtime errors when processing invalid or unexpected input types.

    7
    Add support for escaped special characters in comments to prevent parsing errors

    The current implementation might incorrectly handle escaped characters within
    comments. Add proper handling of escaped parentheses and semicolons within comments
    to prevent parsing errors.

    src/index.js [91-95]

    +if (char === '\\') {
    +  i++; // Skip next character if escaped
    +  if (openParens > 0) currentComment += char + line[i];
    +  else result += char + line[i];
    +  continue;
    +}
     if (char === ';' && openParens === 0) {
       comments.push(line.slice(i + 1).trim());
       break;
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion improves the robustness of comment parsing by handling escaped characters, which is important for maintaining compatibility with various G-code formats, though not as critical as handling unmatched parentheses.

    6

    @cheton cheton merged commit c4e1efc into master Nov 20, 2024
    3 checks passed
    @cheton cheton deleted the feat/nested-comments branch November 20, 2024 12:46
    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