Repository navigation
Conversation
adamreeve
left a comment
adamreeve
left a comment
There was a problem hiding this comment.
Thanks for addressing this @ump45nose
| using Apache.Arrow; | ||
| using Apache.Arrow.Ipc; | ||
|
|
||
| public static async Task<RecordBatch> ReadArrowAsync(string filename) |
There was a problem hiding this comment.
Can you please add a short description of what this example does. Eg. "Example of reading an Arrow IPC file".
Also, this example only reads the first batch of data from the file. It could mislead people if they copy this code, thinking it will read a whole file. Maybe it should return a List<RecordBatch> instead?
There was a problem hiding this comment.
Thanks @adamreeve. In 27ade26 the example now starts with "Example of reading all record batches from an Arrow IPC file", and it reads every batch into a List<RecordBatch> until ReadNextRecordBatchAsync() returns null, so copying it no longer silently drops data. (AI-assisted reply.)
The package README caused the Bash verification jobs on Linux, macOS, and Windows to request a nonexistent README.md NuGet package. Iterate only source directories and preserve package names containing spaces. Validated with bash -n, whitespace checks, and eight isolated enumeration assertions covering baseline packages, README files, spaces, and no package directories. Full .NET and RC validation was not run locally. AI-assisted change.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
What changed in this PR
Adds a NuGet-specific README and configures packing so all packable projects include it, eliminating dotnet nuget push “missing readme” warnings.
Changes:
- Add
src/README.mdtailored for nuget.org (packages list, usage snippet, links). - Wire up
PackageReadmeFileand pack the README via sharedDirectory.Build.props. - Adjust RC verification script to iterate
srcdirectories more directly when fetching NuGet artifacts.
| File | Description |
|---|---|
| src/README.md | Adds a NuGet-facing README with package overview, usage example, and links. |
| dev/release/verify_rc.sh | Updates artifact-fetch loop to iterate ../src/*/ directories. |
| Directory.Build.props | Sets PackageReadmeFile and packs src/README.md into all packable NuGet packages. |
Review effort: Lite
| <Content Include="$(CSharpDir)logo_asf.png" Link="logo_asf.png" Pack="true" PackagePath="\" /> | ||
| <Content Include="$(CSharpDir)src/README.md" Link="README.md" Pack="true" PackagePath="\" /> |
| for package in ../src/*/; do | ||
| [ -d "${package}" ] || continue | ||
| package="${package%/}" | ||
| package="${package##*/}" |
| RecordBatch batch; | ||
| while ((batch = await reader.ReadNextRecordBatchAsync()) != null) | ||
| { | ||
| batches.Add(batch); | ||
| } |
What's Changed
dotnet nuget pushwarns that our packages have no readme. This adds a short package readme (src/README.md) and wires it up in the shared NuGet properties inDirectory.Build.props(PackageReadmeFileplus a packedContentitem next to the license and icon), so every packable project undersrc/gets it.The root
README.mdis aimed at people building the repo (build steps, relative links to source folders), so I wrote a separate, smaller one for nuget.org: what Apache Arrow is, the list of packages, a short IPC reading example and links to the docs and the repository. All links are absolute so they work on nuget.org. It carries the usual ASF license header like the other Markdown files.Checked with
dotnet pack -c Releasefor each project insrc/: all seven.nupkgfiles now containREADME.mdat the package root and their nuspec has<readme>README.md</readme>, with no new pack warnings.This PR was prepared with AI assistance and reviewed by me.
Closes #45.