-
Notifications
You must be signed in to change notification settings - Fork 54
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
chore: flatter exports, docs fixes and README header #609
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Robert Pack <[email protected]>
interesting, docs build seems to fail now, while it did work locally... I'll see whats going on. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #609 +/- ##
=======================================
Coverage 83.45% 83.45%
=======================================
Files 74 74
Lines 16877 16877
Branches 16877 16877
=======================================
Hits 14084 14084
Misses 2135 2135
Partials 658 658 ☔ View full report in Codecov by Sentry. |
@@ -16,14 +16,14 @@ use crate::engine::arrow_data::ArrowEngineData; | |||
use crate::{DeltaResult, FileDataReadResultIterator, FileMeta}; | |||
|
|||
/// A fallible future that resolves to a stream of [`RecordBatch`] | |||
/// cbindgen:ignore | |||
// cbindgen:ignore |
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.
//
comments are unceremoniously stripped out by the parser and disappear.
///
comments become annotations that cbindgen can actually see and work with.
I agree it's unfortunate that this pollutes the public docs, tho.
Signed-off-by: Robert Pack <[email protected]>
Signed-off-by: Robert Pack <[email protected]>
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.
thanks! just one thought about the html in the readme
@@ -1,3 +1,29 @@ | |||
<p align="center"> |
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.
mmm, this does look nice rendered by github. I don't love dumping html into our readme though. also the align attribute is deprecated in html5, so maybe not the best to use in new stuff?
I guess I don't generally love assuming people will only view this on github in the browser. I'd like cat README.md
to work okay, which maybe it still does, but certainly not quite as well.
thoughts?
If we gave up centering could the rest be done in "normal" markdown?
What changes are proposed in this pull request?
Some small docs fixes. E.g. while docs.rs seems to render links fine when the reference is also wrapped backticks - [
something
]: ... - rust analyzer does not recognize this, and requires a "plain" name.The architecture and roadmap document in the
doc/
folder seemed grossly outdated and of little value, so I removed them.Also wanted to propose some flair for the Readme :).
This PR affects the following public APIs
While there are no actual API changes, some structs can now be imported (IMO) more conveniently. Building out the datafusion integration, I found myself frequently having to write deep imports - e.g.
How was this change tested?
current tests + rendering and inspecting docs with