-
Notifications
You must be signed in to change notification settings - Fork 0
Gyan/fix sentence case issues #255
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
Conversation
Reviewer's Guide by SourceryThis pull request addresses sentence case formatting issues in headings across the documentation and code. It updates the heading processing logic in Updated class diagram for format-headings.tsclassDiagram
class format-headings.ts {
+ABBREVIATIONS: string[]
+restoreAbbreviations(text: string, replacements: string[][]): string
+toSentenceCase(text: string): string
+processLine(line: string): string
}
note for format-headings.ts "ABBREVIATIONS now includes: HSM, API, SDK, URL, JSON, GraphQL"
note for format-headings.ts "processLine now correctly handles asterisks and numbered headings"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @gyan-sharma - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes to
format-headings.tslook good, but could be improved by adding a unit test to verify the new regex. - Consider whether the abbreviation list in
format-headings.tsshould be configurable.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Updated regex to handle all cases including spaced asterisks | ||
| const headingRegex = | ||
| /^(#{1,6}\s*\*{0,2}\s*(?:\d+(?:\.\d+)?\.?\s*)?)([a-zA-Z])(.*?)$/; | ||
| const match = line.match(headingRegex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider simplifying the heading extraction by breaking the regex into clearer pieces and moving the heading processing into its own helper function to improve readability and reduce complexity
The change introduces a regex that uses additional captures and logic for reassembling the heading, which makes the code harder to follow. You can simplify the extraction by breaking the regex into clearer pieces and by moving the heading processing into its own helper. For example:
function parseHeading(line: string) {
// Break the line into three groups: heading markers, prefix (including any numbering or asterisks), and the content.
const parts = line.match(/^(#{1,6})(\s*\*{0,2}\s*(?:\d+(?:\.\d+)?\.?\s*))?(.*)$/);
if (!parts) return null;
const [, hashes, prefix = "", content] = parts;
return { hashes, prefix, content };
}
function processLine(line: string): string {
if (!line.startsWith("#")) return line;
const parsed = parseHeading(line);
if (parsed) {
const { hashes, prefix, content } = parsed;
return `${hashes}${prefix}${content.charAt(0).toUpperCase()}${content.slice(1)}`;
}
return line;
}This refactoring isolates the regex parsing, makes the purpose of each capture clear, and reduces the complexity of the inline logic.
Summary by Sourcery
Fix sentence case and abbreviation handling in documentation and formatting scripts
Bug Fixes:
Enhancements:
Documentation:
Chores: