GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — url,tools,benchmark: replace deprecated `substr()` by pluris · Pull Request #51546 · nodejs/node

Conversation

@pluris

@pluris pluris commented Jan 22, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Jan 22, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot 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
MrJithil approved these changes Jan 23, 2024
View reviewed changes

@MrJithil MrJithil left a comment

Copy link
Copy Markdown
Member

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@anonrig

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.
MrJithil and pluris reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pluris
pluris force-pushed the fix/depre_url branch 2 times, most recently from 7242e8c to 879629c Compare January 23, 2024 03:52
@pluris

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pluris
pluris force-pushed the fix/depre_url branch from 879629c to 6e2fe0e Compare January 23, 2024 06:22
marco-ippolito
marco-ippolito approved these changes Jan 23, 2024
View reviewed changes
lpinca
lpinca approved these changes Jan 23, 2024
View reviewed changes
hulkish
hulkish reviewed Jan 23, 2024
View reviewed changes
Comment thread lib/url.js Outdated

@hulkish hulkish Jan 23, 2024

Copy link
Copy Markdown

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@lpinca lpinca Jan 23, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

srcPathSlashes.charAt(srcPathSlashes.length -1)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hulkish hulkish Jan 23, 2024

Copy link
Copy Markdown

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.

srcPathSlashes.charAt(srcPathSlashes.length -1)

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hulkish hulkish Jan 23, 2024

Copy link
Copy Markdown

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.

Err.. actually just .slice(-1)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pluris reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@pluris pluris Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lpinca
lpinca approved these changes Jan 23, 2024
View reviewed changes
@pluris

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pluris
pluris force-pushed the fix/depre_url branch 2 times, most recently from 1b33e8a to 07f751c Compare January 23, 2024 23:12
@pluris pluris changed the title url: replace deprecated substr() -> substring() url,tools,benchmark: replace deprecated substr() Jan 23, 2024
pluris
url,tools,benchmark: replace deprecated substr()
094c72d
@pluris
pluris force-pushed the fix/depre_url branch from 07f751c to 094c72d Compare February 28, 2024 16:34
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label May 11, 2024
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit 78dbda1 into nodejs:main May 12, 2024
@aduh95

aduh95 commented May 12, 2024

Copy link
Copy Markdown
Contributor

Landed in 78dbda1
pluris reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request May 13, 2024
Jungku Lee @targos
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 <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
Jungku Lee @marco-ippolito
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 <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
Jungku Lee @soophoo
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 <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@pluris @nodejs-github-bot @anonrig @aduh95 @hulkish @lpinca @MrJithil @marco-ippolito
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.