Initial rust implementation #71

JamesFysh · 2017-04-16T19:57:52Z

working implementation (passes all validation, enc/dec, shorten/recover unit-tests); probably room for improvement wrt comments, layout.

Correct the 'repository' field, since I decided to implement this as part of Google' open-location-code repository.

Attempt to get a basic CI build going for rust...

Take all lng/lat coordinates as Point<f64>, rather than as two f64 values.

drinckes

Hi James, thanks for this. I've reviewed and added some comments. Some of them are pretty minor (newlines & formatting), and some are jut shuffling code around. Hopefully not too much!

drinckes · 2017-05-23T13:48:23Z

rust/Cargo.toml

+description = "Library for translating between GPS coordinates (WGS84) and Open Location Code"
+version = "0.1.0"
+authors = ["James Fysh <james.fysh@gmail.com>"]
+license = "MIT/Apache-2.0"


Can you change the license to just Apache-2.0, so it matches the rest of the project?

drinckes · 2017-05-23T13:48:36Z

rust/Cargo.toml

+version = "0.1.0"
+authors = ["James Fysh <james.fysh@gmail.com>"]
+license = "MIT/Apache-2.0"
+repository = "https://github.com/JamesFysh/open-location-code"


Change to point to this repo

drinckes · 2017-05-23T13:49:20Z

rust/Cargo.toml

@@ -0,0 +1,11 @@
+[package]
+name = "olc"


Use a longer package name that more clearly indicates what it is, e.g., "open-location-code".

drinckes · 2017-05-23T13:59:11Z

run_tests.sh

@@ -32,4 +32,12 @@ if [ "$TEST_DIR" == "ruby" ]; then
  exit
 fi

+if [ "$TEST_DIR" == "rust" ]; then
+  curl https://sh.rustup.rs -sSf | sh -s -- -y


AFAICT most other travis configs are using apt-get to fetch and install rust-nightly. Is this possible?

before_install: - yes | sudo add-apt-repository ppa:hansjorg/rust - sudo apt-get update install: - sudo apt-get install rust-nightly

Well, that was painful to implement! I'll have to invest a bit of time in a local setup of TravisCI so I can verify it works before pushing changes, going forward!

Yeah I know, Travis changes can be painful.

drinckes · 2017-05-23T14:03:59Z

rust/src/interface.rs

+
+    // Latitude 90 needs to be adjusted to be just less, so the returned code
+    // can also be decoded.
+    if lat == LATITUDE_MAX {


Using == to compare floats can be optimistic. Maybe use a comparison that the difference between them is very, very small, or that lat is > LATITUDE_MAX? e.g.
if lat > LATITUDE_MAX || (LATITUDE_MAX - lat) < 0.0000000001f64 {

drinckes · 2017-05-23T14:28:56Z

rust/src/codearea.rs

+            self.code_length + other.code_length
+        )
+    }
+}


Add newline.

drinckes · 2017-05-23T14:29:13Z

rust/src/consts.rs

+pub const GRID_ROWS: f64 = 5f64;
+
+// Minimum length of a code that can be shortened.
+pub const MIN_TRIMMABLE_CODE_LEN: usize = 6;


Add newline.

drinckes · 2017-05-23T14:29:34Z

rust/src/interface.rs

+        longitude += area_range;
+    }
+    Ok(encode(Point::new(longitude, latitude), code_area.code_length))
+}


Add newline.

drinckes · 2017-05-23T14:29:55Z

rust/tests/csv_reader.rs

+        }
+        None
+    }
+}


Add newline.

drinckes · 2017-05-23T14:30:25Z

rust/tests/all_test.rs

+
+        tested += 1;
+    }
+    assert!(tested > 0);


Nice check ;-)

- Project name, licensing, repository location - Newlines - Code-style - Corrections to unit-tests - Safer approach to floating-point comparison - Use of constants rather than magic numbers

Having a proper look at this; The rust-nightly package is only build for 14.04. Travis (by default) uses 12.04 (Precise), but has a 14.04 image which you can opt for. Hopefully this doesn't break anything else!

The PPA doesn't have a rust-nightly. It does have a rustc package, and also a separate cargo package. Let's try those!

Not sure why the choice here by apt-get is to fail, rather than pull in the missing dependent package. Oh well

.. Not excited to be putting specific version numbers in. Starting to wish I'd stuck with the rustup approach; it's a little more predicatable!

JamesFysh · 2017-06-30T12:16:17Z

I think I've covered off all the changes you asked for, and (finally) managed to get Travis CI working correctly. Do I need to do anything to confirm that the requested changes have been made?

drinckes · 2017-06-30T12:35:39Z

Thanks @JamesFysh, you don't have to do anything, I don't have time today but I'll merge it early next week. Thanks!

JamesFysh added 9 commits April 17, 2017 05:52

Initial rust implementation

77685cd

working implementation (passes all validation, enc/dec, shorten/recover unit-tests); probably room for improvement wrt comments, layout.

Revise Cargo.toml

52553ca

Correct the 'repository' field, since I decided to implement this as part of Google' open-location-code repository.

TravisCI

33eeb97

Attempt to get a basic CI build going for rust...

Install rust/cargo before using them!

ad2dfed

This time, for sure!

3f11946

Ahh, the joys of working with remote CI

15831b4

Stab in the dark google#13

ee42858

Revise the public interface

8cd36b6

Take all lng/lat coordinates as Point<f64>, rather than as two f64 values.

Ignore rust.iml when publishing

e25df37

drinckes requested changes May 23, 2017

View reviewed changes

JamesFysh added 9 commits June 5, 2017 05:48

Incorporate PR feedback

655ff46

- Project name, licensing, repository location - Newlines - Code-style - Corrections to unit-tests - Safer approach to floating-point comparison - Use of constants rather than magic numbers

Merge remote-tracking branch 'upstream/master'

464ecef

Fix travis unit-tests

37b2c36

Use a PPA that actually exists?

904c44b

TravisCI, again

c0fa5de

Having a proper look at this; The rust-nightly package is only build for 14.04. Travis (by default) uses 12.04 (Precise), but has a 14.04 image which you can opt for. Hopefully this doesn't break anything else!

Helps if we specify actually-existing packages!

2ab0872

The PPA doesn't have a rust-nightly. It does have a rustc package, and also a separate cargo package. Let's try those!

Dependencies

c6ecb22

Not sure why the choice here by apt-get is to fail, rather than pull in the missing dependent package. Oh well

Further deps

019f8b4

.. Not excited to be putting specific version numbers in. Starting to wish I'd stuck with the rustup approach; it's a little more predicatable!

More deps?

5e8614b

drinckes approved these changes Jun 30, 2017

View reviewed changes

drinckes merged commit ee7425e into google:master Jul 4, 2017

drinckes mentioned this pull request Jul 6, 2017

rust setup breaking travis CI #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial rust implementation #71

Initial rust implementation #71

JamesFysh commented Apr 16, 2017

drinckes left a comment

drinckes May 23, 2017

drinckes May 23, 2017

drinckes May 23, 2017

drinckes May 23, 2017

JamesFysh Jun 20, 2017

drinckes Jun 30, 2017

drinckes May 23, 2017

drinckes May 23, 2017

drinckes May 23, 2017

drinckes May 23, 2017

drinckes May 23, 2017

drinckes May 23, 2017

JamesFysh commented Jun 30, 2017

drinckes commented Jun 30, 2017

Initial rust implementation #71

Initial rust implementation #71

Conversation

JamesFysh commented Apr 16, 2017

drinckes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesFysh commented Jun 30, 2017

drinckes commented Jun 30, 2017