-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
repl: catch \v
and \r
in new-line detection
#54512
Conversation
Can you add a test? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54512 +/- ##
==========================================
- Coverage 88.26% 88.24% -0.03%
==========================================
Files 651 651
Lines 183894 183877 -17
Branches 35858 35857 -1
==========================================
- Hits 162315 162255 -60
- Misses 14882 14898 +16
- Partials 6697 6724 +27
|
@RedYetiDev Thanks for the quick fix. |
CC @nodejs/repl |
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.
RSLGTM
Thank you for addressing this
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.
RSLGTM.
Maybe this'll fix it? |
Could someone start a new CI? Triagers can't start one without an approving review. |
CI LGTM. This should be good to |
Commit Queue failed- Loading data for nodejs/node/pull/54512 ✔ Done loading data for nodejs/node/pull/54512 ----------------------------------- PR info ------------------------------------ Title repl: catch `\v` and `\r` in new-line detection (#54512) Author Aviv Keller <redyetidev@gmail.com> (@RedYetiDev) Branch RedYetiDev:patch-106 -> nodejs:main Labels repl, author ready, needs-ci, commit-queue-squash Commits 2 - repl: catch `\v` and `\r` in new-line detection - disable if no inspector Committers 2 - RedYetiDev <38299977+RedYetiDev@users.noreply.github.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/54512 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54512 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - disable if no inspector ℹ This PR was created on Thu, 22 Aug 2024 20:48:50 GMT ✔ Approvals: 3 ✔ - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/54512#pullrequestreview-2258269961 ✔ - Mohammed Keyvanzadeh (@VoltrexKeyva): https://github.com/nodejs/node/pull/54512#pullrequestreview-2259289159 ✔ - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/54512#pullrequestreview-2274130019 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-09-21T15:51:28Z: https://ci.nodejs.org/job/node-test-pull-request/62627/ - Querying data for job/node-test-pull-request/62627/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/11001786432 |
da344b7
to
500c5ee
Compare
I reverted the change suggested because |
CI is 🟢 (yay!) |
Commit Queue failed- Loading data for nodejs/node/pull/54512 ✔ Done loading data for nodejs/node/pull/54512 ----------------------------------- PR info ------------------------------------ Title repl: catch `\v` and `\r` in new-line detection (#54512) Author Aviv Keller <redyetidev@gmail.com> (@RedYetiDev) Branch RedYetiDev:patch-106 -> nodejs:main Labels repl, author ready, needs-ci, commit-queue-squash Commits 4 - repl: catch `\v` and `\r` in new-line detection - disable if no inspector - fixup! - fixup! fixup! Committers 1 - RedYetiDev <38299977+RedYetiDev@users.noreply.github.com> PR-URL: https://github.com/nodejs/node/pull/54512 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54512 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - repl: catch `\v` and `\r` in new-line detection ⚠ - disable if no inspector ⚠ - fixup! ⚠ - fixup! fixup! ℹ This PR was created on Thu, 22 Aug 2024 20:48:50 GMT ✔ Approvals: 4 ✔ - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/54512#pullrequestreview-2258269961 ✔ - Mohammed Keyvanzadeh (@VoltrexKeyva): https://github.com/nodejs/node/pull/54512#pullrequestreview-2259289159 ✔ - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/54512#pullrequestreview-2274130019 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/54512#pullrequestreview-2323267187 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-09-24T15:06:29Z: https://ci.nodejs.org/job/node-test-pull-request/62735/ - Querying data for job/node-test-pull-request/62735/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/11021667751 |
Landed in c6d20a0 |
PR-URL: #54512 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#54512 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Fixes #54504
Uses
RegExpPrototypeExec
(with\n
,\r
, and\v
) rather thanStringPrototypeIndexOf
with\n
.