search

spenserblack / github-stats-rs

A tool for using Github's API
Apache License 2.0
1 stars 5 forks source link

add support for missing metadata #9

Closed slohse closed 5 years ago

slohse commented 5 years ago

Fixes #6 I may have gone a bit overboard with this one...

spenserblack commented 5 years ago

I definitely understand why you'd want to use an enum (I was tempted to do this myself for r#type). But for the sake of simplicity of usage, and consistency, I think it would be better to take a &str for missing_metadata. Probably also rename missing_metadata to just no.

That way users wouldn't have to be familiar with the enum variants, and can just construct a query like below.

Query::new()
    .is("pr")
    .is("merged")
    .no("assignee");

It's really easy for someone to write that, and easy to read that and know that it would convert to is:pr is:merged no:assignee, without looking at the documentation.

I like your idea to use a HashSet to remove duplicates, but I think it's a bit of overkill. We should probably trust anyone using this crate that, if they do add duplicates, they probably wanted to for some reason. I have no idea why they would, but I think that, if they write .no("assignee").no("assignee"), they should get no:assignee no:assignee.

Still, thanks for the PR! Let me know what you think about these suggestions.

slohse commented 5 years ago

Yeah, I was on the fence between the &str approach and the one I went for in the end. But you're right, it's more intuitive the other way. I'll change it. And the name, too - it makes total sense to have it the same as the actual query.

spenserblack commented 5 years ago

Looks great, thanks!