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

Fix: (typescript-vm) - Update getMatchingPathsRecursive parameter… #65

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

iNewLegend
Copy link
Member

@iNewLegend iNewLegend commented Jun 9, 2024

User description

…s in workspace-provider.js (#64)

The parameters for the getMatchingPathsRecursive function in workspace-provider.js have been updated. The root path is combined with the directory name gotten from the packagesPathPattern and the RegExp pattern used in searching the packages is also modified.


PR Type

Bug fix


Description

  • Updated the parameters for the getMatchingPathsRecursive function in workspace-provider.js.
  • Combined rootPath with the directory name from packagesPathPattern.
  • Modified the RegExp pattern used for searching packages to /*/package.json.

Changes walkthrough 📝

Relevant files
Bug fix
workspace-provider.js
Update `getMatchingPathsRecursive` parameters and RegExp pattern

packages/zenflux-typescript-vm/src/providers/workspace-provider.js

  • Updated the parameters for the getMatchingPathsRecursive function.
  • Combined rootPath with the directory name from packagesPathPattern.
  • Modified the RegExp pattern used for searching packages.
  • +2/-2     

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

    …s in `workspace-provider.js` (#64)
    
    The parameters for the `getMatchingPathsRecursive` function in `workspace-provider.js` have been updated. The root path is combined with the directory name gotten from the `packagesPathPattern` and the RegExp pattern used in searching the packages is also modified.
    @iNewLegend iNewLegend merged commit dc7a3c8 into z/cli/feature/registry-suggestions Jun 9, 2024
    3 of 5 checks passed
    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are limited to a single file with a specific focus on updating function parameters and a regular expression. The modifications are straightforward and localized, making the review process relatively quick.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The new RegExp pattern "/*/package.json" might not correctly replicate the original intent of the replaced pattern. The new pattern seems to look for 'package.json' at any level directly under the root, which might not be equivalent to the original dynamic pattern derived from packagesPathPattern.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the regular expression to properly match directories ending with package.json

    The regular expression "//package.json" might not work as intended. It seems to be trying
    to match any directory followed by /package.json, but the correct regex should use .
    to
    match any character (except newline) zero or more times. This will ensure all directories
    ending with package.json are matched.

    packages/zenflux-typescript-vm/src/providers/workspace-provider.js [89]

    -new RegExp("/*/package.json")
    +new RegExp(".*/package.json")
     
    Suggestion importance[1-10]: 9

    Why: The suggested change corrects a potential bug in the regular expression, ensuring that it matches the intended directories. This is crucial for the correct functioning of the path matching logic.

    9
    Best practice
    Replace path.join with path.resolve for better path resolution

    Consider using path.resolve instead of path.join for defining paths. path.resolve will
    handle cases where the path components are absolute paths and will resolve to an absolute
    path, which is generally safer and more predictable in file system operations.

    packages/zenflux-typescript-vm/src/providers/workspace-provider.js [88]

    -path.join(rootPath, path.dirname(packagesPathPattern))
    +path.resolve(rootPath, path.dirname(packagesPathPattern))
     
    Suggestion importance[1-10]: 7

    Why: Using path.resolve can indeed provide more predictable path resolution, especially in cases where absolute paths might be involved. However, this change is more about improving robustness rather than fixing a critical issue.

    7

    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