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

chore: change deprecated string method #258

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Conversation

diegoreis42
Copy link
Contributor

Background Information

  • Fixes issue(s): No
  • Proposed release version: 5.5.2

I have...

  • added at least one test to verify the failure condition is fixed.
  • verified the tests are passing.

While reading the code, I found deprecated warnings. This is not a breaking change, but rather a precaution against potential errors. A clean project like this should not have trivial and annoying warnings.

Sorry, something went wrong.

@mrodrig
Copy link
Owner

mrodrig commented Jul 7, 2024

Thanks for the PR @diegoreis42! I hadn't noticed the deprecation warnings recently, so that's a good catch.

It looks like the tests are failing currently for these changes, and I believe it's because the parameters for substr and substring are slightly different. MDN has some details about the differences here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substring#the_difference_between_substring_and_substr
image
(Edit: Added a screenshot since the link to the specific section wasn't working)

If you're able to update the logic accordingly, I'd be happy to get this merged in and publish a new version with this update.

Thanks again!

@diegoreis42
Copy link
Contributor Author

I'm sorry for the mistake; I only ran the tests for my first change 😅. All tests are now passing.

@mrodrig mrodrig merged commit e26b141 into mrodrig:main Jul 8, 2024
5 checks passed
@coveralls
Copy link

Coverage Status

coverage: 98.015%. remained the same
when pulling fd1871f on diegoreis42:minor-fix
into 84f088b on mrodrig:main.

@mrodrig
Copy link
Owner

mrodrig commented Jul 8, 2024

Thanks @diegoreis42! I'll get this published in 5.5.3 now.

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

Successfully merging this pull request may close these issues.

None yet

3 participants