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

Promoted File#path to IO#path, added path: optional param to IO#new #3275

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

moste00
Copy link
Contributor

@moste00 moste00 commented Sep 21, 2023

Implementing feature Feature #19036] in Ruby 3.2 compatibility.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 21, 2023
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
You need to untag the spec for this with jt untag spec/ruby/core/io/path_spec.rb.

src/main/ruby/truffleruby/core/io.rb Outdated Show resolved Hide resolved
@andrykonchin andrykonchin self-assigned this Sep 22, 2023
@andrykonchin
Copy link
Member

andrykonchin commented Sep 22, 2023

It makes sense to declare in this PR a #to_path method as well, that is just an alias of #path.

@moste00
Copy link
Contributor Author

moste00 commented Oct 5, 2023

It makes sense to declare in this PR a #to_path method as well, that is just an alias of #path.

You mean in IO ? File already declares a to_path using alias_method

@moste00 moste00 force-pushed the compat/Feature#19036 branch from 175a645 to 53dc421 Compare October 5, 2023 14:59
@moste00 moste00 requested review from eregon and andrykonchin October 5, 2023 15:40
@andrykonchin
Copy link
Member

Yes, I mean in IO. CRuby defines #to_path in the IO class:

f = File.new("a.txt", "w")

f.method(:path).owner
#=> IO
f.method(:to_path).owner
#=> IO

@moste00 moste00 force-pushed the compat/Feature#19036 branch from 53dc421 to 958aa07 Compare October 5, 2023 20:25
@andrykonchin
Copy link
Member

Could you please rename the commit, e.g. name it similar to the PR title?

@moste00 moste00 force-pushed the compat/Feature#19036 branch from 958aa07 to f1a6eef Compare October 6, 2023 12:28
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Looks good, just a few nits and ready to go

@eregon
Copy link
Member

eregon commented Oct 6, 2023

Could also add a changelog entry? See https://github.com/oracle/truffleruby/blob/master/CONTRIBUTING.md#changelog

@moste00 moste00 force-pushed the compat/Feature#19036 branch from 30cf696 to 328f508 Compare October 6, 2023 17:17
CHANGELOG.md Outdated Show resolved Hide resolved
@andrykonchin
Copy link
Member

Could you please squash working commits? Removing exception classes might deserve a separate commit.

@moste00 moste00 force-pushed the compat/Feature#19036 branch 2 times, most recently from ea41c29 to 03fedec Compare October 28, 2023 13:45
@moste00 moste00 force-pushed the compat/Feature#19036 branch from 03fedec to 8389acd Compare October 28, 2023 17:04
@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label Oct 28, 2023
@graalvmbot graalvmbot merged commit e2b5200 into oracle:master Oct 30, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants