Skip to content

Conversation

Vici37
Copy link
Contributor

@Vici37 Vici37 commented Aug 6, 2025

This is the output of compiling cr-source-typer and running it with the below incantation:

CRYSTAL_PATH="./src" ./typer spec/std_spec.cr \
  --error-trace --exclude src/crystal/ \
  --stats --progress \
  --union-size-threshold 2 \
  --ignore-private-defs \
  src/regex

This is related to #15682 .

This is the output of compiling [cr-source-typer](https://github.com/Vici37/cr-source-typer) and running it with the below incantation:

```
CRYSTAL_PATH="./src" ./typer spec/std_spec.cr \
  --error-trace --exclude src/crystal/ \
  --stats --progress \
  --union-size-threshold 2 \
  --ignore-private-defs \
  src/regex
```

This is related to crystal-lang#15682 .
@straight-shoota straight-shoota added this to the 1.18.0 milestone Aug 12, 2025
# re.match("sledding") # => Regex::MatchData("sledding")
# ```
def +(other) : Regex
def +(other : Regex) : Regex
Copy link
Contributor

Choose a reason for hiding this comment

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

That's what Regex.union accepts:

Suggested change
def +(other : Regex) : Regex
def +(other : Regex | String) : Regex

Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit odd though. I don't think the String overload is much useful. Maybe we should deprecate that?

For this PR it might just be best to remove the type restriction. We can follow up with a dedicated change.

Comment on lines 671 to +672
@[Deprecated("Use the overload with `Regex::MatchOptions` instead.")]
def match(str, pos = 0, *, options) : MatchData?
def match(str : String, pos : Int32 = 0, *, options : Regex::Options) : MatchData?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding type restrictions to a deprecated methods?

ditto elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

That's a good catch. Probably not.
However, since it's already there...

@straight-shoota straight-shoota removed this from the 1.18.0 milestone Aug 12, 2025
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