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

feat(stdlib)!: Add an Ascii submodule to Char and move isAscii, toUppercase, toLowercase #2178

Merged
merged 5 commits into from
Nov 7, 2024

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Oct 21, 2024

This pr adds some additional Ascii helper functions to the Char module, it also moves the existing Ascii utilities into a new Ascii submodule within Char.

Breaking: This is a breaking change as isAscii, toAsciiLowercase, toAsciiUppercase, isAsciiDigit and isAsciiAlpha were all moved and renamed within the submodule.

@ospencer
Copy link
Member

I'm in favor of an Ascii submodule.

@spotandjake spotandjake changed the title feat(stdlib): isAscii,isAsciiControl,isAsciiWhitespace to Char feat(stdlib)!: Add an Ascii submodule to Char and move isAscii, toUppercase, toLowercase Oct 28, 2024
@spotandjake
Copy link
Member Author

I'm in favor of an Ascii submodule.

Done

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

This is awesome. Really looking forward to this.

stdlib/char.gr Outdated Show resolved Hide resolved
*
* @example assert Char.isAsciiDigit('1')
* @example assert !Char.isAsciiDigit('a')
* @example Char.Ascii.isAscii('1')
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to do the examples as just Ascii. without the leading Char..

Copy link
Member Author

@spotandjake spotandjake Oct 30, 2024

Choose a reason for hiding this comment

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

I think we should keep the Char.Ascii.isAscii, I feel like it's not very long and it makes the examples runnable, in other modules such as array I was using the pattern:

use Array.{ module Immutable }
assert Immutable.isEmpty(Immutable.empty) == true

but I felt that the module name ascii was small enough here to just directly inline it. I think there is real value in having our examples be as runnable as possible while still being small.

*
* @since v0.7.0
*/
provide let min = 0x00
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be chars? If someone really needs the code then they can do a Char.code on it.

Copy link
Member Author

@spotandjake spotandjake Oct 30, 2024

Choose a reason for hiding this comment

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

We currently use the code on the top level module Char.min. I think if we really want to change that we should do it in a seperate pr after this. Though there is the argument here that min and max are odd values for chars whereas they make more sense in relation to charCodes. maybe we have both minCode, maxCode and min, max?

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. We can leave it as-is.

stdlib/char.gr Outdated Show resolved Hide resolved
spotandjake and others added 3 commits October 30, 2024 12:42
Co-authored-by: Oscar Spencer <oscar.spen@gmail.com>
Co-authored-by: Oscar Spencer <oscar.spen@gmail.com>
@ospencer ospencer added this pull request to the merge queue Nov 7, 2024
Merged via the queue into grain-lang:main with commit 328cf01 Nov 7, 2024
12 checks passed
@github-actions github-actions bot mentioned this pull request Nov 7, 2024
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.

2 participants