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

activesupport: Add types for to_fs #754

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

Copy link

@dak2 Thanks for your contribution!

Please follow the instructions below for each change.
See also: https://github.com/ruby/gem_rbs_collection/blob/main/docs/CONTRIBUTING.md

Available commands

You can use the following commands by commenting on this PR.

  • /merge: Merge this PR if CI passes

activesupport

You changed RBS files for an existing gem.
You need to get approval from the reviewers of this gem.

@hibariya, @ksss, @ydah, @tk0miya, please review this pull request.
If this change is acceptable, please make a review comment including APPROVE from here.
Screen Shot 2024-03-19 at 14 13 36

After that, the PR author or the reviewers can merge this PR.
Just comment /merge to merge this PR.

# 1234567.to_fs(:human, precision: 1,
# separator: ',',
# significant: false) # => "1,2 Million"
def to_fs: (?Symbol format, *untyped) -> String
Copy link
Contributor

Choose a reason for hiding this comment

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

The second argument is a Hash object or kwargs.
https://github.com/rails/rails/blob/v7.0.8.7/activesupport/lib/active_support/core_ext/numeric/conversions.rb#L111

How about this?

Suggested change
def to_fs: (?Symbol format, *untyped) -> String
def to_fs: (?Symbol format, **untyped) -> String
Suggested change
def to_fs: (?Symbol format, *untyped) -> String
def to_fs: (?Symbol format, Hash[untyped, untyped]?) -> String

either is okay to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose **untyped that simply statement. Thanks for pointing that out!

end

# active_support/core_ext/date_time/conversions.rb
class Datetime
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

Suggested change
class Datetime
class DateTime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, my bad! Thanks for noticing. I fixed it.

@tk0miya
Copy link
Contributor

tk0miya commented Dec 18, 2024

Note: #to_fs was introduced (renamed) since 7.0.2.

refs: rails/rails#44354

@dak2 dak2 force-pushed the activesupport/to_fs branch from cb5f911 to 5000fe9 Compare December 19, 2024 09:14
@dak2 dak2 requested a review from tk0miya December 19, 2024 09:18
Copy link
Contributor

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

APPROVE

Thank you for your contribution! 🎉

Copy link

Thanks for your review, @tk0miya!

@dak2, @tk0miya This PR is ready to be merged.
Just comment /merge to merge this PR.

@dak2
Copy link
Contributor Author

dak2 commented Dec 19, 2024

/merge

@github-actions github-actions bot merged commit 71fb7ae into ruby:main Dec 19, 2024
6 checks passed
@dak2 dak2 deleted the activesupport/to_fs branch December 19, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants