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

Make tzif public API easier to use #2027

Merged
merged 7 commits into from Jun 9, 2022
Merged

Conversation

nordzilla
Copy link
Member

@nordzilla nordzilla commented Jun 8, 2022

Fixes #1000

@nordzilla nordzilla requested a review from a team as a code owner June 8, 2022 18:19
@nordzilla nordzilla requested a review from sffc June 8, 2022 18:20
@nordzilla
Copy link
Member Author

@sffc Just a small oversight, I merged too soon before publishing and the toml was missing a description.

Manishearth
Manishearth previously approved these changes Jun 8, 2022
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you also want to add a categories key while you're at it?

@nordzilla
Copy link
Member Author

do you also want to add a categories key while you're at it?

Ah, I probably should, probably just ["date-and-time", "parser-implementations"]

I did just publish it, so I'd have to bump to 0.1.1, but that's probably not an issue.

@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • Cargo.lock is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Manishearth
Manishearth previously approved these changes Jun 8, 2022
@nordzilla
Copy link
Member Author

nordzilla commented Jun 8, 2022

Hmm, actually, @sffc @Manishearth I want to get your opinion.

I just tried using the crate externally and it's a bit awkward to use. It requires that you also use the combine crate to have the combine traits available, otherwise you can't parse anything.

There are a couple options:

  1. We could re-export combine, and add some sort of prelude that people can include that has all the traits they need to parse using tzif. I don't really like this, myself.

  2. We could do something along the lines of what Shane suggested in (Move tzif crate from experimental to utils for publishing #2019 (comment)). I wouldn't suggest to create a Parser that implements the traits, otherwise we'd run into the same issue as Transpilation approach #1, but I think we should make a struct that uses the combine traits internally but exposes its own parse() function that doesn't require the downstream user to depend on combine directly.

This seems a lot nicer and easier to use, but it doesn't really give the flexibility of whether you want to use combine::Parse or combine::EasyParse because there are different actual parsers that you can run over the same implementation with combine.

We could get fancy and expose our own enums or generics to still let users choose between Parse and EasyParse or something, but for now I think a simple, opinionated all-in-one solution sound the nicest.

I'd have to bump the version to 0.2.0 but that seems like the best way to go to me.

Sorry, I should have tried using the crate as an external dependency locally before publishing it. Now I know for future crates.

@nordzilla nordzilla changed the title Add decription to TZif crate Make tzif public API easier to use Jun 8, 2022
@nordzilla
Copy link
Member Author

nordzilla commented Jun 8, 2022

@sffc @Manishearth I went ahead and implemented option 2) in the comment above. PTAL

I just made it as plug-and-play (simple) as possible for now. We can always revisit things later if needed.

And I made sure to test it locally as a stand-alone dependency in another test crate:

use tzif::TzifParser;

fn main() {
    let data = TzifParser::parse_file("/usr/share/zoneinfo/America/Los_Angeles").unwrap();
    println!("{:#?}", data);
}

Manishearth
Manishearth previously approved these changes Jun 9, 2022

impl TzifParser {
/// Parses a `TZif` file at the provided `path`.
pub fn parse_file<P: AsRef<Path>>(path: P) -> Result<TzifData, Error> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: Should these just be toplevel public functions (parse_tzif_file and parse_posix_file)?

Typically when I see a Parser object I imagine a mutable object that holds state

current design is mostly fine though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's reasonable.

I'll make the changes.

@nordzilla nordzilla removed the request for review from sffc June 9, 2022 17:07
@nordzilla nordzilla merged commit 38f2736 into unicode-org:main Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse TZif files into ICU4X data formats for TzdbDataProvider
2 participants