Skip to content
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

Refactor: use slice() instead of deprecated substr() to avoid future issues #111

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sarraf1996
Copy link

@sarraf1996 sarraf1996 commented Sep 29, 2024

This Pull Request (PR) is created for issue #110 and needs review to merge it.

What does this PR do?

This pull request replaces all occurrences of the deprecated substr() method with slice() in the codebase.

Why is this needed?

The substr() method is deprecated and may lead to issues or warnings in future JavaScript versions. This update ensures the code remains future-proof and compliant with modern JavaScript standards.

How does it address the issue?

Each instance of substr() has been refactored to slice() to maintain the same functionality.

Are there any side effects?

This change should not introduce any side effects. Both substr() and slice() have similar behavior, with the exception of how they handle negative indices. All instances in the codebase have been reviewed to ensure compatibility.

Additional Context

For more information on the deprecation of substr(), please refer to the MDN documentation: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr.

@sarraf1996
Copy link
Author

@defunctzombie @dougwilson

When you have a moment, could you please review my pull request? I’d appreciate your feedback and any suggestions for improvement.

Thank you for your time and consideration.

Copy link
Member

@IamLizu IamLizu left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@sarraf1996
Copy link
Author

@IamLizu Thanks for reviewing and approving this PR.

As the pull request has been approved, it is now ready for merging. Please feel free to proceed if there are no further concerns.

@gitdevjin
Copy link

gitdevjin commented Oct 2, 2024

Hello, @sarraf1996.
Maybe I am a bit late, but I also reviewed your PR by merging it locally, and tested it. It looks great to me too.
Screenshot 2024-10-02 at 2 19 06 PM

@IamLizu
Copy link
Member

IamLizu commented Oct 4, 2024

@sarraf1996 thank you for being an awesome contributor. Please allow us some time before your PR lands.

@gitdevjin Good work! Do you know that you can check a PR without merging the branch? You can use,

gh pr checkout 111

Here 111 is the PR number in GitHub and gh is a cli tool of GitHub.

@gitdevjin
Copy link

@IamLizu Thanks for the great tip! I didn't know that. I'll definitely check it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants