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: add id to generate file name #33

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Conversation

Romakita
Copy link
Collaborator

@Romakita Romakita commented Oct 29, 2024

BREAKING CHANGE: file are now named by its id (symbolType + symbolName)

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new dashCase function for converting strings to dash-case format.
    • Added an id method in the DocSymbol class for improved identifier generation.
  • Improvements

    • Enhanced asynchronous handling in various methods, including symbol parsing and file writing, improving performance.
    • Implemented pagination in the file scanning process for more efficient file processing.
    • Updated links in documentation for improved accuracy and clarity.
    • Added dependency on lodash for enhanced utility functions.
  • Bug Fixes

    • Adjusted import paths for several modules to ensure proper dependency resolution.
  • Tests

    • Added a test suite for the new dashCase function to validate its functionality.

Copy link

coderabbitai bot commented Oct 29, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

This pull request introduces several modifications primarily focused on import path changes for various modules and the enhancement of asynchronous handling in several methods. Notable updates include the addition of a new dashCase utility function, the introduction of an id getter in the DocSymbol class, and significant changes to the parse, buildApi, and writeSymbol methods to improve their asynchronous processing capabilities. Additionally, a new test suite for the dashCase function has been added to ensure its functionality.

Changes

File Path Change Summary
packages/ts-doc/components/code-highlight/code-highlight.js Updated import path for highlight function from ../../src/highlight to ./highlight.
packages/ts-doc/components/code-highlight/highlight.js Updated import path for context from "../context" to "../../src/context".
packages/ts-doc/components/code-highlight/highlight.spec.js Updated import path for highlight from ./index to ./highlight.
packages/ts-doc/components/symbol-params/symbol-params.js Updated import path for bindSymbols from ../../src/highlight to ../code-highlight/highlight.
packages/ts-doc/src/models/DocSymbol.js Added get id() method to DocSymbol class; modified url and outputPath to use id.
packages/ts-doc/src/parsers/DocParser.js Refactored parse method to use asynchronous processing with Promise.all. Removed unused overview method.
packages/ts-doc/src/scan/scan.js Updated scanFiles method to process files in chunks of five using Promise.all.
packages/ts-doc/src/tasks/build-api.js Refactored buildApi function to use async/await for processing symbols.
packages/ts-doc/src/utils/dashCase.js Added dashCase function for converting strings to dash-case format.
packages/ts-doc/src/utils/dashCase.spec.js Introduced test suite for dashCase function using Chai assertions.
packages/ts-doc/src/write/write.js Updated writeSymbol and writeJson methods to handle asynchronous operations; added a cache to prevent duplicate writes.
packages/ts-doc/src/models/DocSymbol.spec.js Updated expected outputs in tests to reflect changes in URL and output path format.
packages/ts-doc/test/snapshots/ControllerProvider.md Updated links for entity, children, and set children properties in documentation.
packages/ts-doc/test/snapshots/Ignore.md Updated hyperlink reference for IgnoreCallback in documentation.
packages/ts-doc/test/snapshots/RequestContext.md Updated link reference for RequestContextOptions in constructor documentation.
packages/ts-doc/test/snapshots/ReturnsChainedDecorators.md Updated links in method signatures for ReturnsChainedDecorators interface.
packages/ts-doc/package.json Added dependency: "lodash": "^4.17.21" under the dependencies section.

Possibly related PRs

  • fix: add id to generate file name #33: The changes in this PR involve a modification to the import statement for the highlight function in code-highlight.js, which directly relates to the changes made in the main PR regarding the import path for the highlight function.

🐇 In the code, I hop and play,
With imports changed, I find my way.
Asynchronous tasks, oh what a thrill,
Writing symbols with newfound skill.
Dash-case strings, I now can see,
A tidy format, just for me!
Let's celebrate this code ballet! 🎉


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (7)
packages/ts-doc/src/utils/dashCase.js (1)

1-6: Basic implementation looks good but needs additional safeguards.

The regex-based implementation correctly handles basic camelCase to dash-case conversion.

Consider adding these improvements:

 module.exports.dashCase = function dashCase(str) {
+  if (str == null) return '';
+  str = str.trim();
   return str
     .replace(/([a-z])([A-Z])/g, "$1-$2")
     .replace(/[\s_]+/g, "-")
-    .toLowerCase();
+    .toLowerCase()
+    .replace(/^-+|-+$/g, '')  // Remove leading/trailing dashes
+    .replace(/[^a-z0-9-]/g, '-');  // Replace non-alphanumeric with dash
 };
packages/ts-doc/src/utils/dashCase.spec.js (1)

4-9: Enhance test coverage for the dashCase function.

Given that this function is crucial for file naming (breaking change), consider adding more test cases to ensure robust behavior:

  1. Edge cases:
    • Empty string
    • Null/undefined handling
  2. Special patterns:
    • Multiple consecutive uppercase letters (e.g., "myXMLFile")
    • Strings with whitespace/underscores
    • Special characters
  3. More complex scenarios:
    • Mixed case with numbers
    • Leading/trailing special characters

Here's a suggested expansion of the test suite:

 describe("dashCase", () => {
+  it("should handle edge cases", () => {
+    expect(dashCase("")).to.equal("");
+    expect(() => dashCase(null)).to.throw();
+    expect(() => dashCase(undefined)).to.throw();
+  });
+
   it("should convert camelCase to dash-case", () => {
     expect(dashCase("camelCase")).to.equal("camel-case");
     expect(dashCase("camelCase-test")).to.equal("camel-case-test");
   });
+
+  it("should handle special patterns", () => {
+    expect(dashCase("myXMLFile")).to.equal("my-xml-file");
+    expect(dashCase("my_file_name")).to.equal("my-file-name");
+    expect(dashCase("my file name")).to.equal("my-file-name");
+  });
+
+  it("should handle complex scenarios", () => {
+    expect(dashCase("myFile123Name")).to.equal("my-file123-name");
+    expect(dashCase("_myFileName_")).to.equal("my-file-name");
+  });
 });
packages/ts-doc/src/tasks/build-api.js (2)

18-28: Good conversion to async/await, but consider memory and progress handling.

The switch to Promise.all with map is a good improvement for concurrent processing. However, there are a few considerations:

Consider these improvements:

 .then(async () => {
   let symbols = 0;
+  const totalSymbols = context.symbols.toArray().length;
+  const batchSize = 100; // Process symbols in smaller chunks
+  const symbolsArray = context.symbols.toArray();
+  
+  for (let i = 0; i < symbolsArray.length; i += batchSize) {
+    const batch = symbolsArray.slice(i, i + batchSize);
+    await Promise.all(
+      batch.map(async (symbol) => {
+        const content = context.components.page(symbol);
+        symbols++;
+        logger(`Processing symbols: ${symbols}/${totalSymbols}`);
+        return writeSymbol(symbol, content);
+      })
+    );
+  }
-  await Promise.all(
-    context.symbols.toArray().map((symbol) => {
-      const content = context.components.page(symbol);
-      symbols++;
-      return writeSymbol(symbol, content);
-    })
-  );

   logger(chalk.green(symbols) + " symbols write");

This refactoring:

  1. Processes symbols in batches to manage memory usage
  2. Adds progress logging for better visibility
  3. Maintains concurrent processing within each batch

Line range hint 33-34: Consider enhancing error handling.

The catch block currently only logs errors. For a breaking change PR, more robust error handling would be beneficial.

Consider this improvement:

-      .catch((err) => console.error(err));
+      .catch((err) => {
+        logger.error(chalk.red("Failed to build API documentation"));
+        logger.error(err.stack || err);
+        process.exit(1);
+      });
packages/ts-doc/src/write/write.js (1)

Line range hint 41-87: Consider making writeJson consistent with writeSymbol.

The method could benefit from the same improvements made to writeSymbol:

  1. Async/await pattern
  2. Better error handling
  3. Consistent file write flag

Consider this refactor:

-  writeJson() {
+  async writeJson() {
     if (!context.jsonOutputDir) {
       return;
     }
 
     const outFile = path.join(context.jsonOutputDir, "api.json");
     const {version, scope} = context;
 
     const modules = context.symbols.toArray().reduce((acc, symbol) => {
       // ... existing reduce logic ...
     }, {});
 
     const obj = {
       version,
       scope,
       symbolTypes: symbolTypes(),
       symbolStatus: symbolStatus(),
       modules
     };
 
-    fsExtra.writeFileSync(outFile, JSON.stringify(obj, null, 2), {
-      flag: "w+"
-    });
+    try {
+      await fsExtra.writeFile(outFile, JSON.stringify(obj, null, 2), {
+        flag: "w"
+      });
+    } catch (error) {
+      logger.error(`Failed to write api.json: ${error.message}`);
+      throw error;
+    }
     logger(`Write api.json '${chalk.cyan(outFile)}'`);
   }
packages/ts-doc/src/scan/scan.js (1)

65-78: Add progress tracking for better visibility.

For large file sets, it would be helpful to show progress information to users.

     const files = await globby(patterns);
+    const totalFiles = files.length;
+    let processedFiles = 0;
 
     for (let i = 0; i < files.length; i += chunkSize) {
       const filesChunk = files.slice(i, i + chunkSize);
 
       await Promise.all(
         filesChunk.map(async (file) => {
           try {
             const symbols = await DocParser.parse(new DocFile(file));
+            processedFiles++;
+            context.logger(`Progress: ${processedFiles}/${totalFiles} files (${Math.round(processedFiles/totalFiles*100)}%)`);
 
             symbols.forEach((symbol) => {
               context.logger(`Scanned symbol '${chalk.cyan(symbol.symbolName)}'`);
packages/ts-doc/src/parsers/DocParser.js (1)

40-57: Add JSDoc documentation for the async parse method.

The method lacks proper JSDoc documentation describing its async nature and return type.

Add comprehensive JSDoc:

+  /**
+   * Parses the document file and processes its symbols asynchronously
+   * @param {DocFile} docFile - The document file to parse
+   * @returns {Promise<Map<string, Symbol>>} A promise that resolves to a map of processed symbols
+   * @throws {Error} If critical processing errors occur
+   */
   static async parse(docFile) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 84d9702 and 2686ad9.

📒 Files selected for processing (11)
  • packages/ts-doc/components/code-highlight/code-highlight.js (1 hunks)
  • packages/ts-doc/components/code-highlight/highlight.js (1 hunks)
  • packages/ts-doc/components/code-highlight/highlight.spec.js (1 hunks)
  • packages/ts-doc/components/symbol-params/symbol-params.js (1 hunks)
  • packages/ts-doc/src/models/DocSymbol.js (3 hunks)
  • packages/ts-doc/src/parsers/DocParser.js (1 hunks)
  • packages/ts-doc/src/scan/scan.js (1 hunks)
  • packages/ts-doc/src/tasks/build-api.js (1 hunks)
  • packages/ts-doc/src/utils/dashCase.js (1 hunks)
  • packages/ts-doc/src/utils/dashCase.spec.js (1 hunks)
  • packages/ts-doc/src/write/write.js (3 hunks)
✅ Files skipped from review due to trivial changes (2)
  • packages/ts-doc/components/code-highlight/code-highlight.js
  • packages/ts-doc/components/symbol-params/symbol-params.js
🔇 Additional comments (14)
packages/ts-doc/src/utils/dashCase.js (1)

1-6: Verify usage in file name generation.

Let's ensure the function works correctly in the context of file name generation.

✅ Verification successful

Let me gather more information about how dashCase is used in DocSymbol.js since that seems to be the main consumer.


From reviewing the code, I can now generate a final response since I have a clear understanding of how dashCase is used. The function is used in DocSymbol class to generate IDs by combining symbol type and name, which are then used in output file paths.

dashCase function is correctly implemented for file name generation

The function is used in DocSymbol.js for:

  • Generating unique IDs by combining symbol type and name (get id() method)
  • These IDs are used in file paths (get outputPath() method) to create markdown documentation files
  • The implementation correctly handles:
    • camelCase to dash-case conversion
    • Space/underscore to dash conversion
    • Lowercase transformation
    • All necessary characters for safe file names
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find where dashCase is used for file naming
rg -A 5 "require.*dashCase|import.*dashCase"

# Description: Look for file writing/path generation code that might use this
ast-grep --pattern 'writeFile($$$)' 

Length of output: 995


Script:

#!/bin/bash
# Check how dashCase is used in DocSymbol.js
cat packages/ts-doc/src/models/DocSymbol.js

# Also look for any file system operations in DocSymbol.js
rg -A 5 "writeFile|createWriteStream|fs\." packages/ts-doc/src/models/DocSymbol.js

Length of output: 5143

packages/ts-doc/src/utils/dashCase.spec.js (1)

1-2: LGTM!

The test setup correctly imports the required dependencies.

packages/ts-doc/components/code-highlight/highlight.spec.js (2)

Line range hint 4-20: LGTM! Well-structured test case.

The test provides comprehensive coverage of the highlight functionality with detailed assertions for TypeScript syntax highlighting.


1-2: LGTM! More explicit import path.

The change from ./index to ./highlight makes the import more explicit and direct.

Let's verify the module exists at the new location:

✅ Verification successful

Import path change is correct and safe

The verification confirms:

  • The highlight.js file exists and exports the required highlight function
  • No index.js file exists in the directory, suggesting it was renamed to highlight.js
  • The function signature and implementation remain unchanged
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the highlight module exists and is properly referenced
# Expected: The highlight.js file should exist in the same directory

# Check if the highlight.js file exists
fd --type f "highlight.js" --exec echo "✓ Module exists: {}"

# Check for any other files still importing from ./index
rg --type js "require\(['\"]\.\/index['\"]\)" --glob "!node_modules"

Length of output: 398


Script:

#!/bin/bash
# Fix the regex escape sequence and check for index imports
rg --type js "require\(['\"]\./index['\"]" --glob "!node_modules"

# Check if highlight.js exports the highlight function
rg "exports.highlight\s*=" ./packages/ts-doc/components/code-highlight/highlight.js

# Check if there's an index.js file that might be affected
fd --type f "index.js" --parent-dir "packages/ts-doc/components/code-highlight" --exec echo "Found index.js: {}"

Length of output: 596


Script:

#!/bin/bash
# Check if highlight.js exists and exports the highlight function
rg "exports.highlight\s*=" ./packages/ts-doc/components/code-highlight/highlight.js -A 2

# Check for index.js in the code-highlight directory
fd "index.js" --search-path packages/ts-doc/components/code-highlight

# Check for any remaining references to ./index in the codebase
rg "require\(['\"]\.\/index['\"]" packages/ts-doc/components/code-highlight

Length of output: 453

packages/ts-doc/components/code-highlight/highlight.js (2)

Line range hint 4-61: The code highlighting implementation looks good!

The rest of the file remains unchanged and maintains a well-structured implementation of code highlighting with proper use of regular expressions and consistent context object usage.

🧰 Tools
🪛 Biome

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)


3-3: Verify the updated import path.

The import path has been changed from "../context" to "../../src/context". While this change appears to be part of a larger restructuring effort, we should verify that the new path is correct and consistent.

Let's verify the import path and its usage:

✅ Verification successful

Based on the results from both script executions, I can now generate the final response:

The updated import path ../../src/context is correct and consistent with the codebase structure.

The directory structure shows that:

  • The file is located at packages/ts-doc/components/code-highlight/highlight.js
  • The context module is in packages/ts-doc/src/context/
  • Other files in the codebase consistently import from the context module using relative paths

The import path change from ../context to ../../src/context is correct as it properly reflects the relative path from the component's location to the context module.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the context module's location and its usage across the codebase

# Test 1: Check if the context module exists at the new location
fd --type f "context.{js,ts}" src/

# Test 2: Check for any other imports of the context module to ensure consistency
rg "require\(.*context.*\)" --type js --type ts

# Test 3: Check if there are any other files in the same directory using the old import path
fd --type f ".*\.(js|ts)$" "packages/ts-doc/components/code-highlight/" --exec rg "require\(.*context.*\)"

Length of output: 1939


Script:

#!/bin/bash
# Let's verify the actual location of the context module and its structure

# Check if context exists in the packages/ts-doc directory structure
fd --type f "context.{js,ts}" packages/ts-doc/

# Check the directory structure of packages/ts-doc to understand the paths
fd --type d . packages/ts-doc/ --max-depth 3

# Look for the main context file content to understand its location
rg --type js --type ts -l "^module.exports.*=.*context" packages/ts-doc/

Length of output: 1312

packages/ts-doc/src/write/write.js (2)

16-16: LGTM! Good improvements to file writing logic.

The changes improve the code by:

  1. Making operations asynchronous
  2. Preventing duplicate writes with cache
  3. Using correct file write flag

Also applies to: 21-27, 32-34


48-48: LGTM! ID field addition aligns with PR objectives.

The addition of the id field to the JSON output properly implements the breaking change described in the PR objectives.

Also applies to: 60-60

packages/ts-doc/src/scan/scan.js (2)

61-78: LGTM! Good improvement in file processing efficiency.

The implementation of chunked processing with Promise.all is a solid improvement that will help prevent memory issues when dealing with large file sets.


67-73: Verify error recovery for partial chunk failures.

The current implementation continues processing other files when one fails, which is good. However, we should verify that the error handling in upstream code (DocParser.parse) properly cleans up resources.

✅ Verification successful

Let me search for error handling patterns in DocParser more broadly to understand the resource management better.


Let me try one more search to find all try-catch blocks with proper regex escaping.


Resource cleanup is properly handled - no issues found

After analyzing the codebase, I can confirm that the error handling is appropriate and there are no resource cleanup concerns:

  1. DocFile operations use Node.js built-in fs module which automatically handles file descriptor cleanup
  2. DocParser.parse() implements error handling that:
    • Catches and logs errors for individual symbol processing
    • Continues processing other symbols when one fails
    • Does not hold any resources that need explicit cleanup

The error recovery strategy is well-implemented, allowing the scanning process to continue even when individual symbols fail to parse.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for resource cleanup in error handling within DocParser
ast-grep --pattern 'try {
  $$$
} catch ($_) {
  $$$
  cleanup($_)
  $$$
}'

Length of output: 77


Script:

#!/bin/bash
# Search for DocParser implementation and error handling
ast-grep --pattern 'class DocParser {
  $$$
}'

# Also check for any try-catch blocks in DocParser
rg -A 5 "try.*{" --type js packages/ts-doc/src/

Length of output: 11299


Script:

#!/bin/bash
# Search for try-catch blocks in the codebase
rg -A 5 "try \{" --type js packages/ts-doc/src/

# Also check for any file handling or resource management
rg -A 5 "new DocFile" --type js packages/ts-doc/src/

Length of output: 4158

packages/ts-doc/src/parsers/DocParser.js (1)

40-57: Verify the impact of async changes on callers.

The method has been changed to async, which might affect calling code that expects synchronous behavior.

Let's check for potential breaking changes:

✅ Verification successful

Let me check the caller's implementation to verify if it's already handling the async behavior correctly.


No changes required - caller already handles async behavior correctly

The caller in scan.js is already using await with DocParser.parse() and is properly handling the asynchronous nature of the method within an async function. The code is wrapped in Promise.all() and uses async/await syntax correctly, indicating it was designed for asynchronous operation from the start.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for direct calls to DocParser.parse
rg -l "DocParser\.parse\(" | xargs rg -A 2 "DocParser\.parse\("

# Search for potential imports of DocParser
rg -l "require.*DocParser" | xargs rg "DocParser\.parse"

Length of output: 340


Script:

#!/bin/bash
# Get the implementation context of the caller
rg -B 5 -A 5 "DocParser\.parse" packages/ts-doc/src/scan/scan.js

Length of output: 438

packages/ts-doc/src/models/DocSymbol.js (3)

5-5: LGTM: Import statement is correctly placed and follows conventions.


134-134: Verify impact of breaking change in file naming.

This modification changes the output file naming convention from using just symbolName to using the new id format. This is a breaking change that could affect:

  1. External tools or scripts that depend on the file naming convention
  2. Documentation links that reference these files
  3. Existing documentation builds

Let's analyze the potential impact:

#!/bin/bash
# Check for existing MD files that might need migration
echo "Searching for existing markdown files that might need migration..."
fd -e md . || echo "No markdown files found"

# Check for potential hardcoded references to .md files
echo "Checking for hardcoded references to .md files..."
rg "\.md['\"]" || echo "No hardcoded .md references found"

49-51: 🛠️ Refactor suggestion

Consider adding input validation for symbolType and symbolName.

While the implementation is correct, it might be vulnerable to runtime errors if either symbolType or symbolName is undefined/null. Consider adding validation:

  get id() {
+   if (!this.symbolType || !this.symbolName) {
+     throw new Error('Symbol type and name are required for id generation');
+   }
    return dashCase(this.symbolType + "-" + this.symbolName);
  }

Let's verify the current usage of these properties:

packages/ts-doc/src/utils/dashCase.js Outdated Show resolved Hide resolved
packages/ts-doc/src/write/write.js Show resolved Hide resolved
packages/ts-doc/src/write/write.js Outdated Show resolved Hide resolved
packages/ts-doc/src/scan/scan.js Show resolved Hide resolved
packages/ts-doc/src/parsers/DocParser.js Outdated Show resolved Hide resolved
packages/ts-doc/src/parsers/DocParser.js Show resolved Hide resolved
packages/ts-doc/src/parsers/DocParser.js Outdated Show resolved Hide resolved
@Romakita Romakita force-pushed the fix-generate-id-per-symbol branch 2 times, most recently from 4b4f10c to 7d7fda5 Compare October 29, 2024 07:02
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (2)
packages/ts-doc/src/write/write.js (1)

Line range hint 83-85: Consider consistency improvements.

  1. The writeJson method remains synchronous while writeSymbol is async. Consider making both methods consistently async.
  2. The "w+" flag is unnecessary for write-only operations. Consider using "w" as done in writeSymbol.
-    fsExtra.writeFileSync(outFile, JSON.stringify(obj, null, 2), {
-      flag: "w+"
-    });
+    await fsExtra.writeFile(outFile, JSON.stringify(obj, null, 2), {
+      flag: "w"
+    });
packages/ts-doc/src/models/DocSymbol.js (1)

49-51: Add JSDoc documentation for the id getter.

Since this is a breaking change that affects file naming, it would be helpful to add JSDoc documentation explaining the format and purpose of the generated ID.

+  /**
+   * Generates a unique identifier by combining the symbol type and name.
+   * Used for generating consistent file names in the documentation.
+   * @returns {string} A dash-case formatted identifier
+   */
   get id() {
     return dashCase([this.symbolType, this.symbolName].filter(Boolean).join("-"));
   }
🛑 Comments failed to post (5)
packages/ts-doc/src/write/write.js (1)

21-26: ⚠️ Potential issue

Consider potential race conditions in cache checks.

The cache check and add operations are not atomic. In concurrent scenarios, this could lead to race conditions where multiple writes occur despite the cache.

Consider using a more robust synchronization mechanism or moving the cache check closer to the write operation:

-    if (cache.has(symbol.outputPath)) {
-      return;
-    }
-
-    cache.add(symbol.outputPath);
+    // Atomic check-and-set using a temporary flag
+    if (!cache.has(symbol.outputPath)) {
+      cache.add(symbol.outputPath);

Committable suggestion was skipped due to low confidence.

packages/ts-doc/src/models/DocSymbol.spec.js (1)

29-29: 💡 Codebase verification

Add test cases for different symbol types to ensure consistent ID generation

Based on the codebase analysis, we found that symbolType can be:

  • "function" (default)
  • "const"
  • "decorator" (when in decorators directory)
  • "service" (when in services directory)
  • "interface" (special handling for privacy)

Add test cases for each symbol type to verify:

  1. The ID generation with symbolType-symbolName format
  2. The special directory-based type transformations (function → decorator/service)
  3. The privacy handling for different types (especially interface)

Example:

it("should generate correct IDs for different symbol types", () => {
  const cases = [
    { type: "function", name: "foo", expected: "function-foo" },
    { type: "const", name: "BAR", expected: "const-bar" },
    { type: "interface", name: "ITest", expected: "interface-itest" }
  ];
  
  cases.forEach(({type, name, expected}) => {
    const symbol = new DocSymbol();
    symbol.symbolType = type;
    symbol.symbolName = name;
    expect(symbol.id).toBe(expected);
  });
});
🔗 Analysis chain

Add test cases for different symbol types.

While the test covers the "Const" symbol type, we should add test cases for other possible symbol types to ensure the ID generation works correctly across all cases.

Let's check what other symbol types exist:

Consider adding a dedicated test for the id getter to explicitly verify the combination of symbolType and symbolName.


The previous search didn't yield the expected results. Let's try to find the actual implementation of DocSymbol class and any type definitions that might indicate the possible symbol types.


From the DocSymbol implementation, we can see that symbolType can be "function", "decorator", "service", "const", and "interface". Let's verify if there are more types defined in the context or assigned directly in the code.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find all possible symbol types used in the codebase
rg -g '*.ts' -g '*.js' 'symbolType.*=.*["'\''](\w+)["'\'']' -o --no-filename | sort -u

Length of output: 8422


Script:

#!/bin/bash
# Let's try a different approach to find symbol types
# First, let's look for the DocSymbol class implementation
ast-grep --pattern 'class DocSymbol {
  $$$
}'

# Also search for any enum or type definition related to symbol types
rg -g '*.ts' -g '*.js' '(type|enum)\s+.*Symbol(Type|Kind)' -A 5

Length of output: 12586


Script:

#!/bin/bash
# Looking at the implementation, we can see symbolType is modified in setDocFile method
# Let's search for context.symbolTypes to find the available types
rg -g '*.js' -g '*.ts' 'context\.symbolTypes\s*=' -A 10

# Also search for where symbolType is directly assigned
rg -g '*.js' -g '*.ts' 'symbolType\s*=\s*["'\''](\w+)["'\'']' --only-matching

Length of output: 474

packages/ts-doc/src/models/DocSymbol.js (2)

120-120: 🛠️ Refactor suggestion

Consider using path.join for URL construction.

While the current implementation works, using path.join would be more robust for handling path separators consistently.

-    const url = [context.baseUrl, normalizePath(path.dirname(this.docFile.relativePackagePath)), `${this.id}.html`].join("/");
+    const url = normalizePath(path.join(context.baseUrl, path.dirname(this.docFile.relativePackagePath), `${this.id}.html`));

Committable suggestion was skipped due to low confidence.


49-51: 💡 Codebase verification

Breaking change detected in ID generation that requires attention

The verification reveals two critical issues that need to be addressed:

  • symbol-header.js directly uses symbolName for ID generation with a different format: symbol.symbolName.replace(/ /gi, "").toLowerCase()
  • The new id getter is used in URL and file path generation, which will create different paths than before since it now includes the symbolType prefix

This change will break existing documentation links and file paths. The implementation needs to be aligned with the existing ID generation logic or requires a migration strategy.

Locations needing attention:

  • packages/ts-doc/components/symbol-header/symbol-header.js: Uses incompatible ID generation
  • packages/ts-doc/src/models/DocSymbol.js: New ID format affects url and outputPath getters
🔗 Analysis chain

Implementation looks good, but verify impact of breaking change.

The id getter implementation is clean and handles edge cases well. However, since this is marked as a breaking change, we should verify its impact on existing documentation.

Let's check for existing documentation files that might be affected:


Let me check how the symbolName is used in the documentation templates and test snapshots to better understand the impact.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all existing documentation files to assess the impact
# of the new naming convention

# Look for existing documentation files
fd -e md -e html . packages/ts-doc/

# Check if there are any direct references to the old naming pattern
rg -l "symbolName" packages/ts-doc/

Length of output: 1035


Script:

#!/bin/bash
# Check usage in EJS templates and test snapshots
rg -C 2 "symbolName" packages/ts-doc/components/symbol-summary/symbol-summary.ejs packages/ts-doc/components/symbol-header/symbol-header.js packages/ts-doc/test/snapshots/*.md

# Check how the id is currently used
rg -C 2 "\.id" packages/ts-doc/src/

Length of output: 2309

packages/ts-doc/test/snapshots/RequestContext.md (1)

36-36: 💡 Codebase verification

Interface links in documentation need to be updated to use consistent dash-case format

Several interface links in the documentation files still use the old format without dashes. These need to be updated to match the new dash-case convention:

  • packages/ts-doc/test/snapshots/ReturnsChainedDecorators.md:
    • /api/schema/interfaces/interface-json-header.html
    • /api/schema/interfaces/interface-json-headers.html
  • packages/ts-doc/test/snapshots/Ignore.md:
    • /api/schema/interfaces/interface-ignore-callback.html
🔗 Analysis chain

Link update aligns with the new file naming convention.

The updated link to RequestContextOptions follows the new naming convention using dash-case format, which is consistent with the PR's objective of improving file name generation.

Let's verify the consistency of similar interface links in the documentation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining old-style interface links in markdown files
# that haven't been updated to the new dash-case format

# Search for interface links in markdown files that don't use the dash-case format
rg -g '*.md' '/api/.*interface[^-].*\.html'

# Search for the new format to confirm the pattern
rg -g '*.md' '/api/.*interface-.*\.html'

Length of output: 10900

BREAKING CHANGE: file are now named by its id (symbolType + symbolName)
@Romakita Romakita force-pushed the fix-generate-id-per-symbol branch from 7d7fda5 to ebd1698 Compare October 29, 2024 07:06
@Romakita Romakita merged commit f03a453 into master Oct 29, 2024
4 checks passed
@Romakita
Copy link
Collaborator Author

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

1 participant