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

Emit comments for each node in a node list (microsoft:#45776) #45794

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

@hyrumcoop
Copy link

@hyrumcoop hyrumcoop commented Sep 9, 2021

Fixes #45776

@microsoft-cla
Copy link

@microsoft-cla microsoft-cla bot commented Sep 9, 2021

CLA assistant check
All CLA requirements met.

Loading

// ];
if (mayEmitInterveningComments && format & ListFormat.CommaDelimited) {
if (emitTrailingCommentsOfPosition) {
if (child.kind !== SyntaxKind.PropertyAssignment) {
Copy link
Member

@andrewbranch andrewbranch Sep 9, 2021

Choose a reason for hiding this comment

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

Why do PropertyAssignments need to be filtered out?

Also nit: these ifs can be collapsed into one.

Loading

Copy link
Author

@hyrumcoop hyrumcoop Sep 9, 2021

Choose a reason for hiding this comment

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

Duplicate comments were being emitted before filtering out PropertyAssignments. I believe PropertyAssignment comments are already being emitted by emit(child), which is why it would make sense to filter here. But it does feel like a workaround, and I'm new to the TypeScript codebase so I'm unsure of a more correct way to go about this is.

And thanks for the suggestion on the if-statements, I will fix that shortly.

Loading

Copy link
Member

@andrewbranch andrewbranch Sep 9, 2021

Choose a reason for hiding this comment

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

@rbuckton will likely have a suggestion about whether there’s a better place for this to go where it wouldn’t have to filter like this, or maybe a better condition to filter by. I’m not very familiar with comment emit myself.

Loading

@sandersn sandersn added this to Not started in PR Backlog Sep 21, 2021
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
PR Backlog
  
Waiting on reviewers
Linked issues

Successfully merging this pull request may close these issues.

4 participants