-
Notifications
You must be signed in to change notification settings - Fork 44
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
Update documentation to include write examples #100
Conversation
Can someone review? 🙂 cc @lnicola |
README.md
Outdated
}); | ||
|
||
// Create file at path | ||
let gpx_file = File::create(out_path); |
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.
let gpx_file = File::create(out_path); | |
let gpx_file = File::create(out_path)?; |
README.md
Outdated
gpx.tracks[0].segments[0].points.push(Waypoint::new(geo_point)); | ||
|
||
// Write to file | ||
write(&gpx, gpx_file?)?; |
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.
write(&gpx, gpx_file?)?; | |
gpx::write(&gpx, gpx_file)?; |
README.md
Outdated
`write` will write the GPX output to anything that implements `std::io::Write`. To save the output to a string, use a mutable `BufWriter` to write it to a vector, and then convert the vector to a string. | ||
```rust | ||
let mut buf = BufWriter::new(Vec::new()); | ||
write(&gpx, &mut buf)?; // writes to buf |
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.
write(&gpx, &mut buf)?; // writes to buf | |
gpx::write(&gpx, &mut buf)?; // writes to buf |
README.md
Outdated
``` | ||
|
||
### Write to string | ||
`write` will write the GPX output to anything that implements `std::io::Write`. To save the output to a string, use a mutable `BufWriter` to write it to a vector, and then convert the vector to a string. |
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 don't think you need to wrap a Vec
in a BufWriter
. But it might be a good idea to do it for files, since write
might cause very short writes. So can you update both examples?
README.md
Outdated
|
||
pub fn to_gpx<P: AsRef<Path>>(out_path: P) -> Result<String, Box<dyn Error>> { | ||
// Instantiate Gpx struct | ||
let mut gpx: Gpx = Gpx { |
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.
Let's build this in the other order (let track_segment = ...; let track = ...; let gpx = ...
). That way wou don't need indexing and can do directly e.g. vec![track]
.
README.md
Outdated
use gpx::{Gpx, Track, TrackSegment, Waypoint, Route}; | ||
use geo_types::{Point, coord}; | ||
|
||
pub fn to_gpx<P: AsRef<Path>>(out_path: P) -> Result<String, Box<dyn Error>> { |
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.
pub fn to_gpx<P: AsRef<Path>>(out_path: P) -> Result<String, Box<dyn Error>> { | |
pub fn to_gpx<P: AsRef<Path>>(out_path: P) -> Result<(), Box<dyn Error>> { |
Thanks for the review, great suggestions. I'm new to this library and Rust generally, so I'll defer to your suggestions. I'll push the changes in the next couple of days, hopefully by the end of this coming weekend. |
README.md
Outdated
pub fn to_gpx<P: AsRef<Path>>(out_path: P) -> Result<String, Box<dyn Error>> { | ||
// Instantiate Gpx struct | ||
let mut gpx: Gpx = Gpx { | ||
version: GpxVersion::Gpx11, // or Gpx10 |
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'm personally not a fan of these type annotations / comments here. If you're just skimming the README, it's enough to know you can set e.g. the waypoints
, but the exact type won't matter too much. And if you've pasted the code into your editor, you'll hopefully get enough info from it.
Ok, code has been updated! May you take a look when you get a chance, @lnicola? |
@@ -2,6 +2,7 @@ | |||
|
|||
## Unreleased | |||
|
|||
- [#100](https://github.com/georust/gpx/pull/100): Add write examples to `README.md` |
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.
Oh, my, do we really have so many unreleased changes?
README.md
Outdated
```rust | ||
use std::path::Path; | ||
use gpx::{Gpx, Track, TrackSegment, Waypoint, Route}; | ||
use geo_types::{Point, coord}; |
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're missing a couple of imports:
use geo_types::{coord, Point};
use gpx::{Gpx, GpxVersion, Track, TrackSegment, Waypoint};
use std::{error::Error, fs::File, io::BufWriter, path::Path};
README.md
Outdated
// Write to file | ||
gpx::write(&gpx, buf)?; | ||
|
||
Ok(()); |
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.
Ok(()); | |
Ok(()) |
README.md
Outdated
``` | ||
|
||
### Write to string | ||
`write` will write the GPX output to anything that implements `std::io::Write`. To save the output to a string, write it to a UTF-8 vector, and then convert the vector to a string. |
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.
`write` will write the GPX output to anything that implements `std::io::Write`. To save the output to a string, write it to a UTF-8 vector, and then convert the vector to a string. | |
`write` will write the GPX output to anything that implements `std::io::Write`. To save the output to a string, write it to an `u8` vector, and then convert the vector to a `String`. |
README.md
Outdated
### Write to string | ||
`write` will write the GPX output to anything that implements `std::io::Write`. To save the output to a string, write it to a UTF-8 vector, and then convert the vector to a string. | ||
```rust | ||
let mut vec: Vec<u8> = Vec::new(); |
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.
let mut vec: Vec<u8> = Vec::new(); | |
let mut vec = Vec::new(); |
Done :-) ... can you take a look again @lnicola ? |
Thanks and sorry for the back and forth! bors r+ |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
No worries, thanks for the comments, it's helping me learn good practices in Rust 🙂 |
CHANGELOG.md
if knowledge of this change could be valuable to users.Fixes #98