Skip to content

Add commentValue property to Trivia for cleaned comments #2966

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zyadtaha
Copy link

This PR continues the work from #2578, addressing the review comments and refining the behavior of Trivia.commentValue.

- Copied changes from PR swiftlang#2578 (swiftlang#2578)
- Original work by @adammcarter
- Manual copy due to inaccessible original branch
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the issue, @zyadtaha. I think the PR still needs some polishing regarding indentation handling. I left a few comments inline.

@zyadtaha zyadtaha force-pushed the trivia-piece-comment-value branch from 3ad450f to c23e3dc Compare February 16, 2025 10:07
@zyadtaha
Copy link
Author

Thanks for the feedback, @ahoppen! I've addressed the comments—let me know if anything else needs attention.

@zyadtaha zyadtaha requested a review from ahoppen February 17, 2025 08:15
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks for the adjustments. I left a few more edge cases to consider as comments.

Add release note for Trivia.commentValue

Address PR review

Address second review
@zyadtaha zyadtaha force-pushed the trivia-piece-comment-value branch from c23e3dc to 04f700e Compare February 21, 2025 13:16
@zyadtaha zyadtaha requested a review from ahoppen February 21, 2025 13:17
@zyadtaha
Copy link
Author

Hey @ahoppen, just wanted to check if there's anything else needed from my side on this PR. Let me know if you have any concerns. Thanks!

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. I think there are still a few more issues that need to be addressed.

@zyadtaha
Copy link
Author

Hey @ahoppen, I've pushed my updates. For your last two comments, I've added responses—let me know your feedback when you get a chance. Thanks!

@zyadtaha zyadtaha force-pushed the trivia-piece-comment-value branch from 8df001d to 52fdc83 Compare April 19, 2025 15:19
@zyadtaha zyadtaha requested a review from ahoppen April 19, 2025 15:21
@ahoppen
Copy link
Member

ahoppen commented Apr 23, 2025

I addressed my own review comments in a 2197103, which I pushed to your branch. Could you check if these changes make sense to you?

@zyadtaha
Copy link
Author

I addressed my own review comments in a 2197103, which I pushed to your branch. Could you check if these changes make sense to you?

Thank you! I pulled the changes and reviewed the new logic and tests, and everything looks great to me. I really appreciated the detailed doc comments; they helped me understand your approach much better. I also liked how you split the tests into separate functions since it makes things much clearer.

I just had one question: why did you move the commentValue property to a separate file, and similarly, the test cases to a separate file?"

Also, if you have any feedback on my code, communication, or anything I could improve, I’d be very grateful. I want to make sure I’m not wasting your time and that I'm learning as much as possible through this process.

@ahoppen
Copy link
Member

ahoppen commented Apr 29, 2025

Thank you! I pulled the changes and reviewed the new logic and tests, and everything looks great to me. I really appreciated the detailed doc comments; they helped me understand your approach much better. I also liked how you split the tests into separate functions since it makes things much clearer.

Thanks for checking that the changes look good to you.

I just had one question: why did you move the commentValue property to a separate file, and similarly, the test cases to a separate file?"

The implementation of commentValue ended up being non-trivial and since it isn’t all that related to the other Trivia functionality (which is a lot more low-level), I decided to put it into its own file. This way we can also communicate that the extensions defined in that file are only intended for the commentValue property.

Also, if you have any feedback on my code, communication, or anything I could improve, I’d be very grateful. I want to make sure I’m not wasting your time and that I'm learning as much as possible through this process.

You did great work to get things started. I just figured that it was easier to fix the edge cases instead of repeatedly pointing out edge cases and then discovering more after you addressed them. Thinking of all possible edge cases is a big part of developing compiler-related tooling and it’s hard to teach that general way of thinking in pull request comments.

@ahoppen ahoppen requested a review from rintaro April 29, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants