-
Notifications
You must be signed in to change notification settings - Fork 22
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
Publish v0.1.0 to crates.io #45
Comments
🎉 We use a very close fork of this repo, the only mandatory thing we'd need from a published crate to use it is some public modules : |
Hey, thanks for asking! I have a few thoughts on things that might be good to add before an initial release in order to prevent major breaking changes for consumers down the road, and also make feature additions and enhancements a little easier as well. Decouple filesystem implementation from parsing functionsIn // traits.rs
use std::io::Read;
pub trait FileProvider {
fn tracev3_files(&self) -> Box<dyn Iterator<Item=Box<dyn Read>>>;
fn uuidtext_files(&self) -> Box<dyn Iterator<Item=Box<dyn Read>>>;
fn dsc_files(&self) -> Box<dyn Iterator<Item=Box<dyn Read>>>;
fn timesync_files(&self) -> Box<dyn Iterator<Item=Box<dyn Read>>>;
} This could also be written to allow for static dispatch, but would require consumers to use pub trait FileProvider {
fn tracev3_files(&self) -> impl Iterator<Item=impl Read>;
fn uuidtext_files(&self) -> impl Iterator<Item=impl Read>;
fn dsc_files(&self) -> impl Iterator<Item=impl Read>;
fn timesync_files(&self) -> impl Iterator<Item=impl Read>;
} I lean towards the first option since I view the cost of using dynamic dispatch with Then, a type can be defined that implements that trait, providing users with an easy path to the common case while allowing loose coupling for users to implement the trait in whatever way works best for them. If we want to allow for, for instance, on-demand lookups of strings down the road (I believe @jrouaix proposed a more memory friendly implementation of This could be as simple as wrapping use std::path::PathBuf;
struct LogArchive(PathBuf);
impl FileProvider for LogArchive {
// implement trait methods
} Allow for variation in behavior for certain aspects of the parserThe big one here that I'm thinking of is I have far less understanding of this part of the code than you, but my reading of the source suggests that this should be possible. Decide on public API + add docsWhat would really help this release go off with a bang is if we updated the docs and decided exactly what should be public and why. It may also be a good idea to re-export some things with All of that is a quite a lot in order for this to be released to crates.io on the 19th, so I totally understand if you don't feel like trying to hit all of those items. I could also be off target in my understanding of some things, so if anyone has better ideas for how these things could be acheived I'd love to hear them. For my part, I'd be happy to implement at least the first point in the next few days, though, and hopefully contribute to the docs as well. |
Wow @dgmcdona very nice backlog ! It will need a really long time to achieve all this. Some more thoughts :
|
thanks for the comments all. The 19th date was deadline if no comments were posted on issue. I'm fine adjusting date if needed, but I think since the library is an ok working state, im inclined to release a 0.1.0 version on 19th (or this weekend). IMO releasing a version on crates.io now would allow users who are comfortable using the library in its current state to have a stable version to use (instead of specific a github repo commit in Cargo.toml). While major changes are being done in preparation for version 1.0 (or 0.2.0). Regarding comments so far:
|
Thanks for the feedback! I have a working implementation of the filesystem decoupling that I can open an MR for (at least a draft one) to get feedback on tomorrow evening, and I’d be happy to flesh out the docs as well once we finalize a design that we’re happy with. Releasing to crates.io so users can pin a stable version makes sense to me as well, especially since it’s still only v0.1.0 and people should expect breaking changes as the library matures anyway. I don’t feel like that should necessarily block on any of those issues I raised, but it may be worth taking a look at the aforementioned PR to see if it’s something you’d want to include, since it does a lot for usability but will definitely be a breaking change to the most public-facing part of the API. |
Now that this project has been tested on lots of log files and receiving feedback (and additional contributions).
It makes sense to start publishing to crates.io, this will hopefully make it easier to use in projects.
@jrouaix @dgmcdona you both have been making alot of contributions, anything either of you want to get merged before first version gets published to crates?
Otherwise, i will try to publish on 2024-12-19 🥳
The text was updated successfully, but these errors were encountered: