Improve tag api #25
Improve tag api #25
Conversation
src/objects.rs
Outdated
| /// Returns the key associated value or `None` if there is no such tag. | ||
| pub fn get(&self, key: &str) -> Option<&str> { | ||
| self.0.get(key).map(|v| v.as_str()) | ||
| } |
There was a problem hiding this comment.
get is already usable, returning a Option<&String>
There was a problem hiding this comment.
It's easy to turn a String into a &str, however it's very annoying to have in one code &"s".to_owned(). So I definitely prefer it this way. :)
There was a problem hiding this comment.
Give me the context where this method is way better than the already existing one, please.
There was a problem hiding this comment.
It's very useful in case you don't want to deconstruct a type. For example:
fn foo(s1: &String) {
Some(s1) == Some("b"); // doesn't work!
}
fn main() {
foo(&"a".to_owned());
}There was a problem hiding this comment.
Better to add a doccomment on Tags saying that it Deref to a Map<String, String>?
There was a problem hiding this comment.
Too sad there is not the "Methods from Deref<Target = TargetImpl>" as in the official documentation https://doc.rust-lang.org/std/string/struct.String.html
There was a problem hiding this comment.
Oh so I wasn't completely crazy! I'll check if I can make it visible. But I still think that we should remove contains.
There was a problem hiding this comment.
But I still think that we should remove contains.
I think we can't agree on that ;-)
There was a problem hiding this comment.
Well, you're the maintainer so I'll leave it there. But please note that I complained about it. :)
src/objects.rs
Outdated
| } | ||
| /// Returns `true` if contains a tag with the given `key`. | ||
| pub fn contains(&self, key: &str) -> bool { | ||
| self.0.get(key).is_some() |
There was a problem hiding this comment.
No, because:
- that's a breaking change
- that's not the idiomatic API: you can already do
tags.contains_key("foo")
This contains method is here because that's something you want a lot, and is easier to write than your get variant. It follows the Set interface.
There was a problem hiding this comment.
that's a breaking change
I changed the medium version of the crate.
that's not the idiomatic API: you can already do
tags.contains_key("foo")
That's a very good point, it should indeed be contains_key. Documentation failed me.
This
containsmethod is here because that's something you want a lot, and is easier to write than yourgetvariant. It follows theSetinterface.
I don't agree with this assumption: having a get is more idiomatic. contains is misleading in my opinion (that's mainly why I didn't think of contains_key).
There was a problem hiding this comment.
You already have get and contains_key via Deref
There was a problem hiding this comment.
Then this method shouldn't be there.
GuillaumeGomez
force-pushed
the
improve-tag-api
branch
from
055e251 to
28364a8
Compare
|
Ok, deref methods now appear in the documentation. |
GuillaumeGomez
force-pushed
the
improve-tag-api
branch
from
28364a8 to
209211f
Compare
|
I also made a minor crate version change instead of a medium one. |
GuillaumeGomez
force-pushed
the
improve-tag-api
branch
from
209211f to
f1010d7
Compare
| @@ -17,30 +17,33 @@ use std::ops::{Deref, DerefMut}; | |||
| /// tags](http://wiki.openstreetmap.org/wiki/Tags) for more | |||
| /// information. | |||
| #[derive(Debug, Default, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] | |||
| pub struct Tags(TagsImpl); | |||
| /// FlatMap representing the key-value pairs of the tags | |||
| pub type TagsImpl = ::flat_map::FlatMap<String, String>; | |||
There was a problem hiding this comment.
You must let this type alias, else it's a breaking change.
There was a problem hiding this comment.
It doesn't change the API, so from my point of view it isn't one.
There was a problem hiding this comment.
All codes using your crate won't break with this change.
There was a problem hiding this comment.
It will if someone use something like struct Data { tags: osmpbfreader::objects::TagsImpl }
There was a problem hiding this comment.
Good point. Do you mind if we remove it then?
There was a problem hiding this comment.
I'd prefer to keep it, but no strong feeling on it (I doubt someone is using it).
| pub struct Tags(TagsImpl); | ||
| /// FlatMap representing the key-value pairs of the tags | ||
| pub type TagsImpl = ::flat_map::FlatMap<String, String>; | ||
| pub struct Tags(::flat_map::FlatMap<String, String>); |
There was a problem hiding this comment.
Maybe just not using the type alias here would be enough to have the documentation. Looks like a bug, no?
There was a problem hiding this comment.
And the alias is useless, unless I'm missing something?
There was a problem hiding this comment.
The alias is more here for backcompatibility, before, this TagsImpl was a new type if I recall well.
GuillaumeGomez
force-pushed
the
improve-tag-api
branch
from
f1010d7 to
adf0af4
Compare
|
v0.13.3 published. |
|
Thanks! :) |
|
I opened the PR to rustdoc as well: rust-lang/rust#68093 |
No description provided.