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

Improve docstrings for filesize and stat #56244

Merged

Conversation

KronosTheLate
Copy link
Contributor

The docstring for filesize failes to mention how paths are joined, and document the full-path-as-single-argument method. I know that this version of the docstring might be a regression when it comes to accurately reflecting the underlying methods, but it does serve the propose of documenting how it should be called much better, which is IMO the purpose of docstrings.

I also updated the docstring for stat to drop the shortcut, and be more explicit about the intended format for arguments to be slurped and stating that joinpath is used.

base/stat.jl Outdated Show resolved Hide resolved
@nsajko nsajko added the docs This change adds or pertains to documentation label Oct 19, 2024
@DilumAluthge DilumAluthge changed the title Improve docstring filesize Improve docstrings for filesize and stat Oct 20, 2024
Co-authored-by: Neven Sajko <[email protected]>
@LilithHafner
Copy link
Member

We should probably keep filesize in sync with all of

ispath, isfifo, ischardev, isdir, isblockdev, isfile, issocket, issetuid, issetgid, issticky, uperm, gperm, operm, filemode, filesize, mtime, and ctime.

They are all defined here:

julia/base/stat.jl

Lines 487 to 507 in 6c70bf7

for f in Symbol[
:ispath,
:isfifo,
:ischardev,
:isdir,
:isblockdev,
:isfile,
:issocket,
:issetuid,
:issetgid,
:issticky,
:uperm,
:gperm,
:operm,
:filemode,
:filesize,
:mtime,
:ctime,
]
@eval ($f)(path...) = ($f)(stat(path...))
end

@KronosTheLate
Copy link
Contributor Author

KronosTheLate commented Nov 3, 2024

We should probably keep filesize in sync with all of ...

That turned out to be a bigger job than I expected. I ended up putting in the work to clean up the docstrings throughout the module. Here is a highlight of the changes made, and relevant desicions:

  • I changed file to path in all docstrings where the intended usage is passing a path
  • I added signatures for stat_struct, which replaces the signatures that take in file, as it is more explicit on how to pass in a "file". This can confuse newer users, but felt important to accurately and completely document potential usage of the function. This signature comes last to minimize that potential confusion.
    • I did not add signatures for stat_struct for functions that are clearly intended to operate on paths rather than file objects/descriptors, namely the functions isfile, ispath, isdir, issocket, and islink. I figured it would only add confusion, and that for these functions, the method that take StatStructs as inputs are best considered an implementation detail rather than valid usage of the function.
  • I cleaned up the language in terms of accuracy, such as changing "Return true if path is a symbolic link" into "Return true if path points to a symbolic link". The original version was plain wrong when taken literally, as path is a string and not a symbolic link. This makes the language a bit more verbose, but makes it so that when you read each word carefully you get the right conclusion, which was impossible before without also making the right assumptions.
  • The original PR mentions the internal usage of joinpath in the docstring. Adding this to each and every docstring felt like too much, and the functionname(path_elements...) signature shoud document this sufficiently. I have kept the explicit statement "If multiple arguments are given, they are joined by joinpath" in the docstring for stat, so that if a user keeps drilling down, they will reach the stat function and can read it there. Adding the stat_struct signatures, along with the new and improved docstring for StatStruct, makes it more possible to understand that stat is used internally in all cases, which is why it felt adequate to mention joinpath explicitly only once, and in the docstring for stat
  • Added more context and examples to the docstring for uperm.
  • Changed the docstring for the functions that directly access fields from the StatStruct from e.g. "Equivalent to stat(file).ctime." to "Equivalent to stat(path).ctime or stat_struct.ctime.", being more clear on the distinction between file and path.
  • At least one punctuation, and one plural-replated gramatical error.

Copy link
Member

@LilithHafner LilithHafner 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! This is clearly a significant improvement and I appreciate the work you put into it. I agree with all your changes and have a few minor tweaks I'd recommend.

base/stat.jl Outdated Show resolved Hide resolved
base/stat.jl Outdated Show resolved Hide resolved
base/stat.jl Outdated Show resolved Hide resolved
base/stat.jl Outdated Show resolved Hide resolved
base/stat.jl Outdated Show resolved Hide resolved
base/stat.jl Outdated Show resolved Hide resolved
KronosTheLate and others added 8 commits November 10, 2024 14:16
Co-authored-by: Lilith Orion Hafner <[email protected]>
Co-authored-by: Lilith Orion Hafner <[email protected]>
Co-authored-by: Lilith Orion Hafner <[email protected]>
Co-authored-by: Lilith Orion Hafner <[email protected]>
Co-authored-by: Lilith Orion Hafner <[email protected]>
base/stat.jl Outdated Show resolved Hide resolved
@LilithHafner
Copy link
Member

Lovely, thanks so much!

@LilithHafner LilithHafner merged commit 5cc5878 into JuliaLang:master Nov 13, 2024
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants