-
Notifications
You must be signed in to change notification settings - Fork 26
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
Improve tag api #25
Improve tag api #25
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>; | ||
pub struct Tags(::flat_map::FlatMap<String, String>); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep: rust-lang/rust#35295 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And the alias is useless, unless I'm missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The alias is more here for backcompatibility, before, this TagsImpl was a new type if I recall well. |
||
|
||
impl Tags { | ||
/// Creates a new, empty `Tags` object. | ||
pub fn new() -> Tags { | ||
Tags(TagsImpl::new()) | ||
Tags(::flat_map::FlatMap::new()) | ||
} | ||
/// Returns if contains the tag `(key, value)`. | ||
/// Returns if it contains the a tag with the given `key` and `value`. | ||
pub fn contains(&self, key: &str, value: &str) -> bool { | ||
self.0.get(key).map_or(false, |v| v.as_str() == value) | ||
} | ||
} | ||
|
||
impl Deref for Tags { | ||
type Target = TagsImpl; | ||
type Target = ::flat_map::FlatMap<String, String>; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
&self.0 | ||
} | ||
} | ||
|
||
impl DerefMut for Tags { | ||
fn deref_mut(&mut self) -> &mut Self::Target { | ||
&mut self.0 | ||
} | ||
} | ||
|
||
impl FromIterator<(String, String)> for Tags { | ||
fn from_iter<T: IntoIterator<Item = (String, String)>>(iter: T) -> Self { | ||
Tags(iter.into_iter().collect()) | ||
|
@@ -69,6 +72,7 @@ pub enum OsmId { | |
/// The identifier of a relation | ||
Relation(RelationId), | ||
} | ||
|
||
impl OsmId { | ||
/// Returns `true` if the id is a node id. | ||
pub fn is_node(&self) -> bool { | ||
|
@@ -82,21 +86,21 @@ impl OsmId { | |
pub fn is_relation(&self) -> bool { | ||
self.relation().is_some() | ||
} | ||
/// Returns the `NodeId` wrapped in an `Option`. | ||
/// Returns the `NodeId` if it is a node, otherwise returns `None`. | ||
pub fn node(&self) -> Option<NodeId> { | ||
match *self { | ||
OsmId::Node(id) => Some(id), | ||
_ => None, | ||
} | ||
} | ||
/// Returns the `WayId` wrapped in an `Option`. | ||
/// Returns the `WayId` if it is a way, otherwise returns `None`. | ||
pub fn way(&self) -> Option<WayId> { | ||
match *self { | ||
OsmId::Way(id) => Some(id), | ||
_ => None, | ||
} | ||
} | ||
/// Returns the `RelationId` wrapped in an `Option`. | ||
/// Returns the `RelationId` if it is a relation, otherwise returns `None`. | ||
pub fn relation(&self) -> Option<RelationId> { | ||
match *self { | ||
OsmId::Relation(id) => Some(id), | ||
|
@@ -123,6 +127,7 @@ pub enum OsmObj { | |
/// A relation | ||
Relation(Relation), | ||
} | ||
|
||
impl OsmObj { | ||
/// Returns the tags of the object. | ||
pub fn tags(&self) -> &Tags { | ||
|
@@ -152,21 +157,21 @@ impl OsmObj { | |
pub fn is_relation(&self) -> bool { | ||
self.relation().is_some() | ||
} | ||
/// Gets a reference to the node in an `Option`. | ||
/// Returns a reference to the `Node` if `self` is a node, otherwise returns `None`. | ||
pub fn node(&self) -> Option<&Node> { | ||
match *self { | ||
OsmObj::Node(ref n) => Some(n), | ||
_ => None, | ||
} | ||
} | ||
/// Gets a reference to the way in an `Option`. | ||
/// Returns a reference to the `Way` if `self` is a way, otherwise returns `None`. | ||
pub fn way(&self) -> Option<&Way> { | ||
match *self { | ||
OsmObj::Way(ref w) => Some(w), | ||
_ => None, | ||
} | ||
} | ||
/// Gets a reference to the relation in an `Option`. | ||
/// Returns a reference to the `Relation` if `self` is a relation, otherwise returns `None`. | ||
pub fn relation(&self) -> Option<&Relation> { | ||
match *self { | ||
OsmObj::Relation(ref r) => Some(r), | ||
|
@@ -189,6 +194,7 @@ pub struct Node { | |
/// The longitude in decimicro degrees (10⁻⁷ degrees). | ||
pub decimicro_lon: i32, | ||
} | ||
|
||
impl Node { | ||
/// Returns the latitude of the node in degrees. | ||
pub fn lat(&self) -> f64 { | ||
|
@@ -212,6 +218,7 @@ pub struct Way { | |
/// The ordered list of nodes as id. | ||
pub nodes: Vec<NodeId>, | ||
} | ||
|
||
impl Way { | ||
/// Returns true if the way is | ||
/// [open](http://wiki.openstreetmap.org/wiki/Way#Open_way). | ||
|
@@ -252,26 +259,31 @@ impl ::std::convert::From<NodeId> for OsmId { | |
OsmId::Node(n) | ||
} | ||
} | ||
|
||
impl ::std::convert::From<WayId> for OsmId { | ||
fn from(w: WayId) -> Self { | ||
OsmId::Way(w) | ||
} | ||
} | ||
|
||
impl ::std::convert::From<RelationId> for OsmId { | ||
fn from(r: RelationId) -> Self { | ||
OsmId::Relation(r) | ||
} | ||
} | ||
|
||
impl ::std::convert::From<Node> for OsmObj { | ||
fn from(n: Node) -> Self { | ||
OsmObj::Node(n) | ||
} | ||
} | ||
|
||
impl ::std::convert::From<Way> for OsmObj { | ||
fn from(w: Way) -> Self { | ||
OsmObj::Way(w) | ||
} | ||
} | ||
|
||
impl ::std::convert::From<Relation> for OsmObj { | ||
fn from(r: Relation) -> Self { | ||
OsmObj::Relation(r) | ||
|
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.
You must let this type alias, else it's a breaking change.
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.
It doesn't change the API, so from my point of view it isn't one.
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.
All codes using your crate won't break with this change.
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.
It will if someone use something like
struct Data { tags: osmpbfreader::objects::TagsImpl }
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.
Good point. Do you mind if we remove it then?
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.
I'd prefer to keep it, but no strong feeling on it (I doubt someone is using it).