FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

url,tools,benchmark: replace deprecated `substr()` by pluris · Pull Request #51546 · nodejs/node · GitHub

/ node Public

url,tools,benchmark: replace deprecated substr() - #51546

Merged
aduh95 merged 1 commit into
nodejs:mainfrom
pluris:fix/depre_url
May 12, 2024
ConversationCommits (1)ChecksFiles changed
Merged

url,tools,benchmark: replace deprecated substr()#51546
aduh95 merged 1 commit into
nodejs:mainfrom
pluris:fix/depre_url

Conversation

pluris commented Jan 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Replace deprecated API.

  • String.prototype.substr() -> String.prototype.substring(), String.prototype.slice()

Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr

nodejs-github-bot commented Jan 22, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url

nodejs-github-bot added needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module. labels Jan 22, 2024
MrJithil approved these changes Jan 23, 2024
View reviewed changes

MrJithil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

anonrig commented Jan 23, 2024

Copy link
Copy Markdown
Member

If we are doing this, we should add a ESLint rule to avoid making the same decision over and over again.

pluris force-pushed the fix/depre_url branch 2 times, most recently from 7242e8c to 879629c Compare January 23, 2024 03:52

pluris commented Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

@anonrig Thank you for your good opinion.
I will update the code about eslint after work.

hulkish reviewed Jan 23, 2024
View reviewed changes
Comment thread lib/url.js Outdated

hulkish Jan 23, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

shouldn't this be srcPathSlashes.charAt(-1) instead of srcPathSlashes.substring(srcPathSlashes.length - 1)?

lpinca Jan 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

srcPathSlashes.charAt(srcPathSlashes.length -1)

hulkish Jan 23, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

srcPathSlashes.charAt(srcPathSlashes.length -1)

hmm I mean sure... but .charAt(-1) accomplishes the same result

hulkish Jan 23, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Err.. actually just .slice(-1)

pluris Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

.charAt(-1) works differently than before. I think .slice(-1) is correct for this case. Thank you for your opinion.

pluris commented Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

I had divided the commits for review, but I will merge them into one and edit the First commit message.

pluris force-pushed the fix/depre_url branch 2 times, most recently from 1b33e8a to 07f751c Compare January 23, 2024 23:12
pluris changed the title url: replace deprecated substr() -> substring() url,tools,benchmark: replace deprecated substr() Jan 23, 2024
aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 11, 2024

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59115/

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59193/

aduh95 merged commit 78dbda1 into nodejs:main May 12, 2024

aduh95 commented May 12, 2024

Copy link
Copy Markdown
Contributor

Landed in 78dbda1

targos pushed a commit that referenced this pull request May 13, 2024
url,tools,benchmark: replace deprecated substr()
ef6a210 
PR-URL: #51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos mentioned this pull request May 13, 2024
Merged
marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
url,tools,benchmark: replace deprecated substr()
be9d6f2 
PR-URL: #51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
url,tools,benchmark: replace deprecated substr()
456b13c 
PR-URL: nodejs#51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
MrJithil MrJithil approved these changes
marco-ippolito marco-ippolito approved these changes
+1 more reviewer
hulkish hulkish left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL