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

Deprecate String#codepoint_at #8475

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Deprecate String#codepoint_at #8475

merged 1 commit into from
Nov 21, 2019

Conversation

vlazar
Copy link
Contributor

@vlazar vlazar commented Nov 15, 2019

Ref #8449

I'm not sure if the use of String#codepoint_at should be removed in this PR or in follow-up PR where String#codepoint_at is removed.

I see some other deprecation warnings when running crystal specs, so I assume it may be intentional and serves as a reminder to remove usage later when the method itself is removed.

Let me know if String#codepoint_at usage should be removed in this PR.

@straight-shoota
Copy link
Member

Deprecated methods should not be used, so please remove its uses in stdlib.

But they continue to be tested in the spec suite, that's where the warnings come from (see discussion in #8454).

@vlazar
Copy link
Contributor Author

vlazar commented Nov 15, 2019

@straight-shoota Sorry, I'm confused now. By stdlib do you mean only the src/ excluding spec/?

The only usage is currently in specs, and those specs are not for codepoint_at itself, it's just that it's being used for testing.

From #8454 (comment)

I prefer a CI with warnings rather than untested deprecated methods. But If I'm the only one who thinks that way, we can go ahead.

So should I remove the usage of codepoint_at from specs in this PR?

@straight-shoota
Copy link
Member

Oh sry, I was under the impression there was a single "real" use somewhere in stdlib.

By stdlib do you mean only the src/ excluding spec/?

Well, essentially all uses except those where specifically this method is tested should be removed. In this case, there are none. But "escapes with unicode" acts as such, so I guess we should leave it for now and only change that with #8476.

@RX14 RX14 added this to the 0.32.0 milestone Nov 21, 2019
@RX14 RX14 merged commit ef7fcd7 into crystal-lang:master Nov 21, 2019
@asterite
Copy link
Member

I'm still not sure why we removed this...

@vlazar
Copy link
Contributor Author

vlazar commented Nov 22, 2019

No real world usage as described in the original issue #8449?

I could only find two real usages of this method in markd, while looking all of GitHub's Crystal repositories.

@asterite
Copy link
Member

I think the number of actual usages don't matter. Crystal is not a hugely popular language. It's good to have the option to easly get a codepoint of a char. That's what makes Ruby great, the many shortcuts you have to things, to avoid boilerplate code or doing many hoops. But if others think removing this was fine then it's fine.

@vlazar
Copy link
Contributor Author

vlazar commented Nov 22, 2019

It's not late yet to revert before the release.

@straight-shoota
Copy link
Member

@asterite Useful shortcuts are great. But codepoint_at(index) doesn't provide a huge benefit over char_at(index).ord. If we follow the discussion from #8449 and rename ord to codepoint, it would at least be a bit shorter, but still not provide a simplification in terms of reducing boilerplate etc.
So you have two ways to do the exact same thing and they are largely identical from a user perspective. Per this argument we try to avoid aliases, and this is essentially the same thing. Removing this method doesn't remove any features because there's an equivalent alternative. But it's one less feature to learn.

vlazar added a commit to vlazar/term_app that referenced this pull request Mar 11, 2020
`String#codepoint_at` is deprecated in Crystal crystal-lang/crystal#8475 and to be removed in this PR crystal-lang/crystal#8476
bcardiff pushed a commit that referenced this pull request Mar 12, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants