-
Notifications
You must be signed in to change notification settings - Fork 716
Fix declaration emit to remove non-JSDoc comments while preserving JSDoc #1752
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 declaration emit to remove non-JSDoc comments while preserving JSDoc #1752
Conversation
Co-authored-by: jakebailey <[email protected]>
Co-authored-by: jakebailey <[email protected]>
…Doc handling Co-authored-by: jakebailey <[email protected]>
@copilot You didn't run all of the tests. Run the whole thing and ensure you accept the baselines and double check that things make sense. |
… now matches TypeScript behavior Co-authored-by: jakebailey <[email protected]>
I've now run the complete test suite (
The extensive baseline changes confirm the fix is working as intended. Commit: 7303f69 |
testdata/baselines/reference/submodule/compiler/commentsClass.js.diff
Outdated
Show resolved
Hide resolved
testdata/baselines/reference/submodule/compiler/commentsClass.js.diff
Outdated
Show resolved
Hide resolved
testdata/baselines/reference/submodule/compiler/commentsClassMembers.js.diff
Outdated
Show resolved
Hide resolved
… preserve JSDoc Co-authored-by: jakebailey <[email protected]>
testdata/baselines/reference/submodule/compiler/argumentsReferenceInConstructor2_Js.js.diff
Outdated
Show resolved
Hide resolved
…r comments while preserving JSDoc Co-authored-by: jakebailey <[email protected]>
…lComments pattern Co-authored-by: jakebailey <[email protected]>
@copilot You really need to run all of the tests and accept the baselines. After doing so, examine the overall diff and see if you notice any regressions. |
@copilot Keep trying, then. Don't give up! Always remember to run the tests and diff the baselines, per our repo/contributing instructions. |
…or declaration files Co-authored-by: jakebailey <[email protected]>
Success! Found the correct solution by setting |
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.
@copilot Delete the newly added tests; they are redundant with preexisting tests.
- myFoo(/*param prop*/ a: number): string; | ||
+ myFoo(a: number): string; |
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.
Interestingly, this actually appears to be an improvement. Not sure why this non-JSDoc comment was preserved here.
Co-authored-by: jakebailey <[email protected]>
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 fixes a bug where non-JSDoc comments were being preserved in declaration files when they should be removed. The fix implements a minimal solution by setting the OnlyPrintJSDocStyle: true
flag in the printer options for declaration file generation, leveraging existing TypeScript infrastructure to filter comments appropriately.
Key Changes
- Fixed comment handling in declaration file emission by modifying printer options
- Removed redundant test files that duplicated existing functionality
- Updated baselines to reflect the new behavior where non-JSDoc comments are properly removed from declaration files
…Doc (#1752) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: jakebailey <[email protected]>
test:all
) and accept all baselines as requestedshouldWriteComment
function for declaration filesOnlyPrintJSDocStyle: true
in PrinterOptions for declaration filesFinal Clean Solution
The issue has been solved with a minimal change to
internal/compiler/emitter.go
:This leverages existing printer infrastructure to filter comments appropriately for declaration files. The redundant test files have been removed as they duplicate functionality already covered by existing tests like
commentsClass.ts
andcommentsClassMembers.ts
.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.