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

A tiny followup for #1641’s oopsies #1672

Merged
merged 4 commits into from Dec 13, 2023
Merged

A tiny followup for #1641’s oopsies #1672

merged 4 commits into from Dec 13, 2023

Conversation

ParadoxV5
Copy link
Contributor

@ParadoxV5 ParadoxV5 commented Dec 11, 2023

  • chomp!: The ? suffix in (?string? separator) is already covered by (nil).
  • scrub: Just like scrub!, scrub(nil, &nil) is a thing.

Question: What shall the convension around ?Regexp | string | nil vs. ?Regexp | string? be?

@ParadoxV5
Copy link
Contributor Author

re-ping: #1641

test `(nil, &nil) -> String`
@@ -1349,7 +1349,7 @@ class String
# modification made, `self` otherwise.
#
def chomp!: (nil) -> nil
| (?string? separator) -> self?
Copy link
Member

Choose a reason for hiding this comment

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

This case is necessary.
Passing a node of type String? will be a type error without the ?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a problem with the type checkers unable to break string and nil from string? to different paths and then merge them back to self? | nil i.e. self?

Copy link
Member

Choose a reason for hiding this comment

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

It may be true. Type checkers can do more about this.
But at least for now, RBS is for static type checking and having string? is more common for static type checking (I believe.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But at least for now

How about this: I will revert it for now, but we should also remind type checker maintainers about these sort of cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Revert the type of chomp!. 🙏

@soutaro soutaro added this to the RBS 3.4 milestone Dec 12, 2023
This PR now only updates `scrub`
@ParadoxV5
Copy link
Contributor Author

No comments? Then how did L648 pass CI

def self.try_convert: (String object) -> String # technically will return `object` unchanged.

@soutaro
Copy link
Member

soutaro commented Dec 12, 2023

@ParadoxV5 You uncovered another bug. 🐞

https://github.com/ruby/rbs/blob/master/lib/rbs/writer.rb#L224 writes lines, from the beginning of end to the end of the method type of the last overload. The comment here is after the end of the method type of the last overload.

Having a comment line between the two overloads would pass CI.

  def chomp!: (nil) -> nil
            # | (?string separator) -> self? # https://github.com/ruby/rbs/pull/1672#discussion_r1423324796
            | (?string? separator) -> self?

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

👍

@soutaro soutaro added this pull request to the merge queue Dec 13, 2023
Merged via the queue into ruby:master with commit 4eb84e2 Dec 13, 2023
24 checks passed
@ParadoxV5 ParadoxV5 deleted the string branch December 13, 2023 03:44
soutaro added a commit that referenced this pull request Dec 20, 2023
This reverts commit 4eb84e2, reversing
changes made to bcf179c.
@soutaro soutaro added the Released PRs already included in the released version label Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released PRs already included in the released version
Development

Successfully merging this pull request may close these issues.

None yet

2 participants