Allowed comment directives to be multiline #38228
Conversation
sandersn
added
the
For Milestone Bug
|
@typescript-bot test this |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 956ac21. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 956ac21. You can monitor the build here. |
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 956ac21. You can monitor the build here. Update: The results are in! |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@DanielRosenwasser Here they are:Comparison Report - master..38228
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
By the way, I realize the turnaround time for my changes on your review comments isn't great - sorry if my responsiveness is holding up getting this in time for 3.9! I wouldn't be offended at all if you added commits to speed it along. |
|
@typescript-bot pack this |
|
Just tried it out on our codebase. :) We still get errors for multiline comments that used the previous recommended workaround.
Though it does now work to change those to be on a single line which looks nicer.
But it would be nice for it to be backwards compatible by handling either format. We have ~919 usages of the old format in our code. And based on the activity here, #27552 it might affect others as well. I'm happy to upgrade our migration tool to use the new format and fix up these usages, but seems like it'd be nice to avoid this as a breaking change by supporting both. Would it be possible to add tests for TSX files as well to help catch this in CI? Thank you for working on this! |
|
😐 sorry, hit the Close button by accident. Anyway, 199d256 should allow any amounts of |
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 65a7587. You can monitor the build here. |
|
Thanks for the quick turnaround @JoshuaKGoldberg! @brieb, a new build should be coming in. |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
|
Confirming that the latest build (from #38228 (comment)) works for us. And, thank you so much for adding TSX tests that would have helped catch the issue! |
|
@JoshuaKGoldberg looks like we need to re-run baselines. After that we can merge. |
|
Well, this is a really cool GitHub feature: ...but the build breaks are unrelated to this PR. |
… multiline-comment-directives
|
I've updated the PR with the fix from master 👍 |
|
Do we need test similar to |
|
@typescript-bot cherry-pick this to release-3.9 |
|
Heya @RyanCavanaugh, I've started to run the task to cherry-pick this into |
|
Hey @RyanCavanaugh, I've opened #38350 for you. |
Component commits: 956ac21 Allowed comment directives to be multiline 12749c9 Added tests, and perhaps fixed a test runner bug? 99bb366 I think it's going to need a consistent variable to loop over a21477d Used dynamically computed indexes in verifies 992441d Added multiline tests 199d256 Increased flexibility for multiline comment parsing 65a7587 Undid a couple of formatting changes; removed backslashes from multiline regexp 036a4ae Merge branch 'master' b620104 Merge branch 'master' of https://github.com/microsoft/TypeScript into multiline-comment-directives
| that could be multiline*/ | ||
|
|
||
| x(); | ||
|
|
There was a problem hiding this comment.
@sheetalkamat sorry, I'm not clear on what you mean by this: #38228 (comment)
Is what you're looking for that a multiline comment containing a directive on its first line should disable the line below it?
If so: sure, I can change the scanner to allow for that. Would you like it to be just the first and last line? Or any line in the comment?
| /*another comment | ||
| that could be multiline*/ | ||
|
|
||
| x(); |
There was a problem hiding this comment.
Yes the question is should this be error or not... Since we are talking about multiline scenarios:: and single line skips the single line comments, should we in general be skipping multiline comments as well
Is better approach to check leading trivia of the node (and or its statement/declaration) where the error is being reported instead?
There was a problem hiding this comment.
That makes sense to me as an improvement point, thanks. Do you think we could push that into a separate issue? I'm wary of continuing to add cherry picking work for TS 3.9.
There was a problem hiding this comment.
@DanielRosenwasser and @RyanCavanaugh will have final say but i think not having that should be ok for 3.9 since multiline ts-ignore is new anyway so its not regression.
Please open new issue to track pending work in that case.
Thanks.
Component commits: 956ac21 Allowed comment directives to be multiline 12749c9 Added tests, and perhaps fixed a test runner bug? 99bb366 I think it's going to need a consistent variable to loop over a21477d Used dynamically computed indexes in verifies 992441d Added multiline tests 199d256 Increased flexibility for multiline comment parsing 65a7587 Undid a couple of formatting changes; removed backslashes from multiline regexp 036a4ae Merge branch 'master' b620104 Merge branch 'master' of https://github.com/microsoft/TypeScript into multiline-comment-directives Co-authored-by: Orta Therox <[email protected]>
Fixes #37738.
Comment directives still keep track of their start and end lines, but are now keyed by the end instead of the start. The last line of multiline comments can also be parsed now.