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

Document Unicode::CaseOptions enum #7513

Merged
merged 5 commits into from
Apr 6, 2019
Merged

Conversation

wooster0
Copy link
Contributor

@wooster0 wooster0 commented Mar 6, 2019

I think the Unicode::CaseOptions enum should be publicly documented when it can be used in methods like String#upcase so you can easily see all the available case options.

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @r00ster91 👍

@asterite
Copy link
Member

asterite commented Mar 7, 2019

Sorry, I don't understand this change. The enum is already documented. And the module too.

@wooster0
Copy link
Contributor Author

wooster0 commented Mar 7, 2019

But not publicy documented. It's not in the API documentation. I think stuff that can be used in public methods (like upcase) should be documented too. When one wants to know the case options he needs to open GitHub first, find unicode.cr by opening src then scroll through till you eventually find the unicode folder, open it, then you perhaps don't know whether it's data.cr or unicode.cr (I don't think that's obvious for everyone) and then you finally see those 3 case options. That's pretty inconvient.

@asterite
Copy link
Member

asterite commented Mar 7, 2019

Oh, I see, this is the relevant line:

require "./unicode/unicode"

src/unicode/unicode.cr Outdated Show resolved Hide resolved
src/unicode/unicode.cr Outdated Show resolved Hide resolved
Co-Authored-By: r00ster91 <r00ster91@outlook.com>
@straight-shoota straight-shoota added this to the 0.28.0 milestone Mar 12, 2019
src/unicode/unicode.cr Outdated Show resolved Hide resolved
@bcardiff bcardiff merged commit df5fde6 into crystal-lang:master Apr 6, 2019
@Blacksmoke16
Copy link
Member

@r00ster91 Am I missing something or is this still not actually visible in the online API docs?

Specifically on https://crystal-lang.org/api/0.28.0/String.html#upcase%28options%3DUnicode%3A%3ACaseOptions%3A%3ANone%29-instance-method, the arg type isnt linked nor is Unicode in the sidebar.

@asterite
Copy link
Member

asterite commented Apr 25, 2019

# :nodoc:
module Unicode

@asterite
Copy link
Member

I guess nobody tried running makes docs to see if the docs appeared?

@Blacksmoke16
Copy link
Member

Apparently not, that would do it.

@Sija
Copy link
Contributor

Sija commented Apr 25, 2019

Seems like :nodoc: from unicode/data.cr has a priority over

# Provides the `Unicode::CaseOptions` enum for special case conversions like Turkic.
module Unicode

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.

8 participants