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

Add parts of methods for Char. #1494

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

SyoujyoujiNaiki
Copy link
Contributor

Related issue: #1487

Copy link

peter-jerry-ye-code-review bot commented Jan 16, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

I'll analyze the git diff and point out potential issues I've observed:

  1. In the is_digit function (around line 90), there seems to be a potential logic issue in the range check:
match radix {
    2..=10 => v >= 48 && v <= radix + 47
    _ => ...
}

The condition v <= radix + 47 might accept characters that shouldn't be valid digits for smaller radixes. For example, for radix 2 (binary), it would accept characters '0' through '9', when it should only accept '0' and '1'.

  1. In the is_control function (around line 84), there's inconsistency in the style of character literals. Some parts of the code use \x notation (like \x7F), while this function uses \u notation (\u0000). For consistency, it might be better to stick to one style throughout the codebase.

  2. The is_ascii_whitespace function (around line 75) might be missing some common whitespace characters, such as \v (vertical tab). Consider whether supporting all ASCII whitespace characters is desired for completeness.

These are potential issues that might need attention, though they may be intentional design choices depending on the requirements of the codebase.

@coveralls
Copy link
Collaborator

coveralls commented Jan 16, 2025

Pull Request Test Coverage Report for Build 4815

Details

  • 17 of 19 (89.47%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 83.146%

Changes Missing Coverage Covered Lines Changed/Added Lines %
char/char.mbt 17 19 89.47%
Totals Coverage Status
Change from base Build 4812: 0.02%
Covered Lines: 4889
Relevant Lines: 5880

💛 - Coveralls

char/char.mbt Outdated Show resolved Hide resolved
char/char.mbt Outdated
(v >= 65 && v <= radix + 65 - 11) ||
(v >= 97 && v <= radix + 97 - 11),
)
_ => None
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does 'This' refer to?

Copy link
Contributor

Choose a reason for hiding this comment

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

he probaly means it should return Bool

Copy link
Collaborator

@peter-jerry-ye peter-jerry-ye Jan 17, 2025

Choose a reason for hiding this comment

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

is_digit('1', 100) == None ==> is_digit('1', 100) == true

_ => None

==>

1 => false
11..<_ => (v >= 48 && v <= 57) ||
        (v >= 65 && v <= radix + 54) ||
        (v >= 97 && v <= radix + 86)

Copy link
Collaborator

@peter-jerry-ye peter-jerry-ye Jan 17, 2025

Choose a reason for hiding this comment

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

or maybe we can simply just panic if you want to follow the Rust's way

Copy link
Collaborator

Choose a reason for hiding this comment

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

@peter-jerry-ye
Copy link
Collaborator

peter-jerry-ye commented Jan 16, 2025 via email

///| Checks if the value is an ASCII uppercase character:
/// U+0041 ‘A’ ..= U+005A ‘Z’
pub fn is_ascii_uppercase(self : Char) -> Bool {
self >= 'A' && self <= 'Z'
Copy link
Contributor

Choose a reason for hiding this comment

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

you can pattern match over char too.

match self {
  'A'..='Z' => true
  _ => false 
}

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.

4 participants