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

Corrected a typo in File's documentation #5933

Merged
merged 1 commit into from Apr 12, 2018
Merged

Corrected a typo in File's documentation #5933

merged 1 commit into from Apr 12, 2018

Conversation

Givralix
Copy link

@Givralix Givralix commented Apr 9, 2018

No description provided.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

While you're at it...

@@ -568,7 +568,7 @@ class File < IO::FileDescriptor
Crystal::System::File.link(old_path, new_path)
end

# Creates a symbolic link at *new_path* to an existing file given by *old_path.
# Creates a symbolic link at *new_path* to an existing file given by *old_path*.
Copy link
Member

Choose a reason for hiding this comment

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

Does old_path really need to exist and be a file?

Copy link
Author

Choose a reason for hiding this comment

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

The function does let you create broken symlinks.

Copy link
Member

Choose a reason for hiding this comment

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

So maybe change it to Creates a symbolic link at *new_path* pointing to *old_path*? But these argument names are not very good either... so maybe it should be reworked in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

More apt would be src_path + dest_path.

Copy link
Member

Choose a reason for hiding this comment

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

Or target and link_name from man ln. But changing the argument names should probably be a separate PR (unless @Givralix want's to tackle this here).

Copy link
Author

Choose a reason for hiding this comment

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

I think it's better if you do it.

@sdogruyol sdogruyol merged commit 7d7e4be into crystal-lang:master Apr 12, 2018
@sdogruyol sdogruyol added this to the Next milestone Apr 12, 2018
@RX14 RX14 added the kind:bug label Apr 12, 2018
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.

None yet

6 participants