Deprecated String.prototype.substr() in robot-name.test.ts

Programming TypeScript
1

Hi! Just got redirected here by the github-actions bot when I opened an issue. Quite a newcomer to open-source contributions, but I’m enjoying Exercism and would love to contribute to the platform in baby-steps :blush: and this was one of the first things I found:

There’s currently 4 instances of String.prototype.substr() in robot-name.test.ts. It’s been deprecated, so I’m thinking it could be refactored to use String.prototype.slice() instead?

Would be happy to open a PR if it were to be updated.

2

The typescript maintainer has had to step away from Exercism for a while. I’m not sure who is around to work with you on this issue.

But it sounds like you’re on the right path to beginning your Exercism contributor journey! Thanks for stepping up.

2 Likes
3

That sounds reasonable to me. I believe Erik would appreciate a PR for this. I’d be happy to review.

4

Great, thank you! Should I post a link to the PR here once done? (not very clear on the workflow just yet)

5

Yes please.

(1) Discuss on the forum. (2) Get a thumbs up for a PR. (3) Create a PR and link here. (4) Profit!

6

Awesome, will do!

:grin:

7

Here’s the PR!

I found that running yarn format also modified 10 other files not related to this PR, so they aren’t included in this commit. They’re the markdown files that are causing the failing check on main right now. Should I add those changes too, or is it for another issue/PR?

8

Thanks for the change!

The cleanest approach would be to push another PR first that does a format cleanup. If those format issues block a push, you can lump them into the same PR. I think Erik prefers fewer PRs over many small PRs.

9

Ahh, yes I agree, it is cleaner for separation of concerns with a separate PR, but I do see why Erik might prefer it bundled into the same PR :sweat_smile:

Don’t mind doing either, but since the changes are quite small and formatting is recommended, should I just commit them too then? Tests, linting and format checks pass on adding them (they’re changing tildes to backticks in the exercism/caution’s and exercism/note’s in the markdown files)

10

Yeah. You can add it to your branch as a new commit and it should show up in the PR.

11

Thanks, I’ve pushed that too. The PR was auto-closed earlier so the latest commit doesn’t show up though

12

The PR was auto-closed earlier so the latest commit doesn’t show up though

When the PR is re-opened, the later commit(s) will show up. All is good.

13

Ah that’s great, it should all be done then. Thanks for guiding me through this! :heart:

1 Like