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

Do not expose Crystal::System::FileInfo in File.info? #11825

Closed
wants to merge 2 commits into from

Conversation

HertzDevil
Copy link
Contributor

Resolves #7685.

@HertzDevil HertzDevil changed the title Do not expose Crystal::System::FileInfo Do not expose Crystal::System::FileInfo in File.info? Feb 14, 2022
Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thanks @HertzDevil 🙏

@straight-shoota
Copy link
Member

What's the point of having a separate Crystal::System::FileInfo type, though? Its API ist just exactly wrapped in File::Info. The only platform-specific aspects are the method implementations and the constructors. The latter can be made :nodoc: and we could just use File::Info directly instead of an identical system type.

@ftarulla
Copy link
Collaborator

Hey @HertzDevil!
Today I mentioned this PR to @beta-ziliani and @straight-shoota.

My understanding after reading this PR and talking to them is that the main problem can be summarized as follows: we need to define the FileInfo API and then we need some platform-specific implementations.

In the current implementation we use inheritance: abstract struct Info would be the API, then leaving its implementation to Crystal::System::FileInfo < ::File::Info (both in unix and win32)
In the proposed solution in this PR, we use composition and so we end up with struct Info and Crystal::System::FileInfo.

A simpler solution (that follows @straight-shoota comment) would be to have some kind of "template class" with the common implementation and also defining the API (File::Info). Then each platform implementation would add the platform-specific methods.

An example of this pattern is the implementation of Socket:

# socket.cr
class Socket < IO
  include Crystal::System::Socket
  ...
  def initialize(...)
    ...
    fd = create_handle(family, type, protocol, blocking)
    ...
  end
end

The implementation of create_handle is platform-specific:

# crystal/system/unix/socket.cr
module Crystal::System::Socket
  private def create_handle(family, type, protocol, blocking) : Handle
    ...
  end
end

Note: The Socket implementation uses modules which I think it won't be necessary in this case.

I know it takes a different approach but would you like to implement the solution I just described?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File.info? returns Crystal::System::FileInfo, not File::Info
5 participants