-
Notifications
You must be signed in to change notification settings - Fork 16
chore(queries): organize and test #366
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Pull Request Overview
This PR addresses issue #356 by refactoring and organizing regex and Unist query utilities into separate, focused modules. The changes improve code organization by extracting regex patterns into a reusable regex module and creating dedicated Unist query functions.
Key Changes
- Extracted all regex patterns from
queries/index.mjs
into a newqueries/regex.mjs
module for better reusability - Created a new
queries/unist.mjs
module containing node type checking functions that were previously inline - Updated all import statements across the codebase to reference the new modular structure
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/utils/queries/regex.mjs |
New module containing all regex patterns with comprehensive documentation |
src/utils/queries/unist.mjs |
New module containing Unist node type checking functions |
src/utils/queries/index.mjs |
Refactored to import and use patterns from new modules, removed inline definitions |
src/utils/parser/index.mjs |
Updated import to use YAML_INNER_CONTENT from regex module |
src/utils/parser/constants.mjs |
Updated to import heading regex patterns from regex module |
Multiple generator and linter files | Updated imports to use functions from the new unist module |
2693be0
to
6bc3f1e
Compare
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.
LGTM ! but could you provide a before after for the issue
Thanks 👍 |
Bump @nodejs/web-infra, this is a bug fix |
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.
Bump @nodejs/web-infra, this is a bug fix
How is such a huge PR a bug fix? This seems a refactor per PR-description. What are you fixing?
@@ -13,6 +13,13 @@ import { | |||
} from '../parser/index.mjs'; | |||
import { getRemark } from '../remark.mjs'; | |||
import { transformNodesToString } from '../unist.mjs'; | |||
import { |
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.
I'd rather a prefixed import
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.
Why?
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.
Cleaner in this situation, these are a bunch on non prefixed constants, doing a wildcard import here better explains where they belong to.
What I fail to understand is why you arbitrarily decided to redesign the structure of the whole queries. Please revert these changes and only include your bug fix. If you want to separate certain things, that's fine, but at least give an explanation why. Arbitrary refactors aren't something I support. |
This PR refactors our RegExp queries, and, in the process, fixes #356, I should not have classified this as a "bug fix", it's a change that also fixes a bug.
I figured, while I'm adding a RegExp file, why not move the query RegExps there as well, and at that point, also move the Unist functions. But I'm happy to revert, since that was quite the tangent. |
I do insist. Not against the refactor, but make a dedicated PR just for the refactor, without the bug fix. This allows us to discuss the bug fix on this PR and the refactor on another :) |
Blocked by #379. TODO Add test cases |
ee35904
to
67904ed
Compare
67904ed
to
2c75bcf
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
+ Coverage 74.27% 76.27% +1.99%
==========================================
Files 118 120 +2
Lines 11041 11906 +865
Branches 695 718 +23
==========================================
+ Hits 8201 9081 +880
+ Misses 2837 2822 -15
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The RegExps have 100% test coverage, with around 600 lines of tests (but I'm open to adding more). Ideally, these RegExps will almost never change, lest we break something |
@@ -99,7 +97,7 @@ function handleExpression(node, basename, nameToLineNumberMap) { | |||
case 'Identifier': { | |||
exports.identifiers.push(value.name); | |||
|
|||
if (CONSTRUCTOR_EXPRESSION.test(value.name[0])) { | |||
if (/^[A-Z]/.test(value.name)) { |
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.
Let's keep regexes in constants for easily finding regexes in one place
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.
Even for these, single-character ones? Or, are you referring to all the RegExps?
The queries and RegExp's we use should be extensively tested to prevent anything slipping through. This PR moves all of our RegExps and queries into their own file, and adds unit tests. This way, we can re-use the same testing structure across our RegExps, to prevent de-syncs.