-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 docs for blob.jl #22555
Add docs for blob.jl #22555
Conversation
base/libgit2/blob.jl
Outdated
|
||
Fetch the *raw* contents of the [`GitBlob`](@ref) `blob`. This is a read-only | ||
`Array` containing the contents of the blob, which may be binary or may be ASCII | ||
`String` data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused a bit by this wording. Is it always just raw bytes which may or may not be valid ASCII? If it's only sometimes ASCII, is that worth noting? Under what circumstances do each of these outcomes occur?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically content
but it won't scream if you try to load raw binary data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be good to just say that? ¯\_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good call.
|
base/libgit2/blob.jl
Outdated
`String` data. | ||
`String` data. `rawcontent` will allow the user to load the raw binary data into | ||
the output `Array` and will not check to ensure it is a valid `String`, so errors | ||
may occur if the result is passed to functions which expect valid `String` data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the result an array or a string? if this is type stable and always array, then you won't be passing it directly to most String functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counterpoint: just a few lines below, s = String(rawcontent(blob))
😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the constructor yes, but not things that expect a String
base/libgit2/blob.jl
Outdated
|
||
See also [`content`](@ref). | ||
See also [`content`](@ref), which *will* throw an error the content of the `blob` | ||
is binary and not valid ASCII. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it error on unicode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think it shouldn't, the libgit2 docs only refer to "printable characters"
OK to merge? |
base/libgit2/blob.jl
Outdated
""" | ||
rawcontent(blob::GitBlob) -> Array | ||
|
||
Fetch the *raw* contents of the [`GitBlob`](@ref) `blob`. This is a read-only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does read only mean here? you can always write to julia arrays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the revised wording make this clearer?
base/libgit2/blob.jl
Outdated
`Array` containing the contents of the blob, which may be binary or may be Unicode. | ||
If you write to this `Array` the blob on disk will not be updated. | ||
`rawcontent` will allow the user to load the raw binary data into | ||
the output `Array` and will not check to ensure it is a valid Unicode, so errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "a valid Unicode" a typical phrase? Perhaps remove "a"?
base/libgit2/blob.jl
Outdated
the output `Array` and will not check to ensure it is valid Unicode, so errors | ||
may occur if the result is passed to functions which expect valid Unicode data. | ||
|
||
See also [`content`](@ref), which *will* throw an error the content of the `blob` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw an error if the content
Ready to roll? |
does git always use utf8 on all platforms? that might be more specific to say, as unicode has multiple encodings |
I don't think so, it appears from some reading that people have used UTF-16 and UTF-32 with git but it's a bit of a wild ride (esp on Windows). |
anything we could test for that? |
I'm not really sure how |
Can we merge this? |
base/libgit2/blob.jl
Outdated
@@ -4,15 +4,28 @@ function Base.length(blob::GitBlob) | |||
return ccall((:git_blob_rawsize, :libgit2), Int64, (Ptr{Void},), blob.ptr) | |||
end | |||
|
|||
""" | |||
rawcontent(blob::GitBlob) -> Array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Vector{UInt8}
(assuming that's true)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
base/libgit2/blob.jl
Outdated
rawcontent(blob::GitBlob) -> Array | ||
|
||
Fetch the *raw* contents of the [`GitBlob`](@ref) `blob`. This is an | ||
`Array` containing the contents of the blob, which may be binary or may be Unicode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"may or may not be valid Unicode"? I find "may be binary or may be Unicode" a little confusing since it's always binary; it doesn't return Char
s.
"valid unicode" is maybe a bit ambiguous still |
Are we ok with the Unicode phrasing? Can we squash? |
|
Anyone? |
I still think "may be binary or may be Unicode" is weird but I don't care enough to make you change it. This LGTM. |
valid utf8 string would make a lot more sense since that describes what the Julia functions will do here |
also xref libgit2/libgit2#4300 |
No description provided.