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

RFC: added chown #15007

Merged
merged 1 commit into from
Feb 23, 2016
Merged

RFC: added chown #15007

merged 1 commit into from
Feb 23, 2016

Conversation

morris25
Copy link
Contributor

@morris25 morris25 commented Feb 9, 2016

If chmod exists why not chown?

@@ -441,3 +442,9 @@ function chmod(p::AbstractString, mode::Integer)
uv_error("chmod",err)
nothing
end

function chown(path::AbstractString, owner::Int, group::Int)
Copy link
Member

Choose a reason for hiding this comment

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

Since -1 leaves things unchanged I think it would make sense to switch group to be an optional parameter. Also owner and group should be Integer.

function chown(path::AbstractString, owner::Integer, group::Integer=-1)
    ...
end

@tkelman
Copy link
Contributor

tkelman commented Feb 9, 2016

Unless this goes through libuv so that it works cross-platform, I see little advantage to wrapping this in a Julia function as opposed to just run(chown ...)

edit: looks like there is a uv_fs_chown so this should use that: https://github.com/JuliaLang/libuv/blob/07730c4bd595b4d45a498a8ee0bcd53878ff7c10/include/uv.h#L1923

@StefanKarpinski
Copy link
Member

I agree that if Base Julia is going to export this it should be portable.

@morris25
Copy link
Contributor Author

I have changed it to use uv_fs_chown and made group optional.
Thanks for the input!

@test_throws Base.UVError chown(file, -1, -2)#on-root user cannot change group to a group they are not a member of (eg: nogroup)
end
: @test chown(file, -2, -2) == nothing#chown shouldn't cause any errors for Windows
)

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be after the end for the test to actually run on windows? would be preferable to use something like if @unix? true : false for blocks rather than extended ternary across lines.

@morris25
Copy link
Contributor Author

I have separated the tests for windows and unix

@@ -81,6 +81,13 @@

Change the permissions mode of ``path`` to ``mode``\ . Only integer ``mode``\ s (e.g. 0o777) are currently supported.

.. function:: chown(path, owner, group =-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the genstdlib script might be sensitive to this matching the signature in the docstring exactly, including spacing

@morris25
Copy link
Contributor Author

I think I've taken care of the documentation fixes now. Please let me know if there is anything else I should change. Thanks!

@tkelman
Copy link
Contributor

tkelman commented Feb 18, 2016

Looks good, can you squash the commits? And minor nitpick but comments look a bit more readable if they are preceded with a space.

@morris25
Copy link
Contributor Author

The commits have now been squashed. Thanks for all the helpful hints!

@morris25
Copy link
Contributor Author

Is there anything else I should fix?

@tkelman
Copy link
Contributor

tkelman commented Feb 23, 2016

Amend the commit message to be a bit more descriptive then I think this is good.

@morris25
Copy link
Contributor Author

Is this better?

@tkelman
Copy link
Contributor

tkelman commented Feb 23, 2016

Better, yeah. I tend to err on the multiline side of longer descriptions when things are nontrivial, but good enough I suppose. Thanks for the contribution!

tkelman added a commit that referenced this pull request Feb 23, 2016
@tkelman tkelman merged commit 199034c into JuliaLang:master Feb 23, 2016
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.

4 participants