Skip to content

Conversation

hanachin
Copy link
Contributor

No description provided.

@hanachin hanachin marked this pull request as ready for review April 30, 2020 16:13
spec.add_development_dependency "rubocop"
spec.add_development_dependency "rubocop-rubycw"
spec.add_development_dependency "minitest-reporters", "~> 1.3.6"
spec.add_development_dependency "json", "~> 2.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

Okay, JSON is a gem... I think the best way is to put RBS in JSON gem, but I think it's okay to have the signatures here for now.

type real = Integer | Float | Rational

type string = String | _ToStr
type encoding = Encoding | string
Copy link
Member

Choose a reason for hiding this comment

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

👍


alias self.fast_unparse self.fast_generate

def self.generate: (_ToJson obj, ?json_options opts) -> String
Copy link
Member

Choose a reason for hiding this comment

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

Could you rewrite these module functions using def self?.generate: ... syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I updated the signature to use the def self?. syntax.

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.

💯

Could you rebase so that I can merge your PR?

@hanachin
Copy link
Contributor Author

hanachin commented May 6, 2020

@soutaro Done ✌️

@soutaro soutaro merged commit c29385a into ruby:master May 7, 2020
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.

2 participants