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

Add signature of FileUtils #576

Merged
merged 3 commits into from Jan 24, 2021
Merged

Add signature of FileUtils #576

merged 3 commits into from Jan 24, 2021

Conversation

ybiquitous
Copy link
Contributor

This change adds signatures for the FileUtils module.

Note: FileUtils has some sub-modules like FileUtils::Verbose, but this change is the first step and does not include such sub-modules.

See also:

@ybiquitous ybiquitous marked this pull request as ready for review January 16, 2021 14:02

type mode = Integer | String

type pathlist = String | Array[String]
Copy link
Member

Choose a reason for hiding this comment

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

I guess it also accepts _ToPath and string.

rbs/core/builtin.rbs

Lines 25 to 27 in e7504e6

interface _ToPath
def to_path: () -> String
end

type string = String | _ToStr

Because most File methods accept them.

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, it is a nice idea!

Also, it seems good to add the following type to core/builtin.rbs, what do you think?

type path = string | _ToPath

Anyway, I will improve the signatures via new following types: 👍

module FileUtils
  type path = string | _ToPath
  type pathlist = path | Array[path]
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed via 929ecc8

Copy link
Member

Choose a reason for hiding this comment

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

Also, it seems good to add the following type to core/builtin.rbs, what do you think?

Personally I think the path type alias should not be defined to the top level. path alias is a domain-specific type, unlike string and so on. So I think it should be defined under a namespace, and I think File::path is appropriate.

And we need to reconsider the other type aliases and interfaces, such as _ToPath, _Reader and so on.

So I'd like to keep them in this pull request, and move them under a namespace by another pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. 👍

# # ... # do something
# end # return to original directory
#
def self.cd: (String dir, ?verbose: boolish) -> void
Copy link
Member

@pocke pocke Jan 17, 2021

Choose a reason for hiding this comment

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

Most (or all?) FileUtils methods are module_function. So let use self? instead of self.

And we need to write aliases for both instance and singleton methods, like the following.

rbs/core/kernel.rbs

Lines 234 to 235 in e7504e6

alias raise fail
alias self.raise self.fail

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! I'll improve the signatures and tests! 💪

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, it might be nice if we could write it as follows... 🤔

-alias self.chdir self.cd
-alias chdir cd
+alias self?.chdir self?.cd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed via ca1d180

Copy link
Member

Choose a reason for hiding this comment

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

BTW, it might be nice if we could write it as follows... thinking

True. I heard the unexistence of self? alias is intentional by @soutaro because of rare.

But maybe we can add self? alias now. Previously I didn't know there are many singleton-instance aliases like that, so I thought self? is unnecessary. But we found demands for singleton-instance aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we found demands for singleton-instance aliases.

Yes, I think so. We could reduce duplications in a case like FileUtils.

Copy link
Member

Choose a reason for hiding this comment

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

@ybiquitous Thanks! I don't think adding new syntax for aliasing module functions makes much sense for now. Let me know if you find this is a frequently seen pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thanks for your comment 👍

Copy link
Member

@pocke pocke left a comment

Choose a reason for hiding this comment

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

Great improvement!

I wrote a few comments, but the other parts look good.

@ybiquitous
Copy link
Contributor Author

@pocke Thank you so much for your advice! I've addressed your comments. 🙆‍♂️

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.

Thank you @ybiquitous ! 👏

@soutaro soutaro merged commit 4fcdf0c into ruby:master Jan 24, 2021
@ybiquitous ybiquitous deleted the add-signature-fileutils branch January 24, 2021 13:22
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.

None yet

3 participants