Refactor native extension by d-unsed · Pull Request #4 · rails-api/case_transform

d-unsed · 2016-09-21T20:28:09Z

Add Transform and TryTransform traits
Move Rust functions outside methods! macro
Change naming to match conventions

d-unsed · 2016-09-21T20:36:55Z

ext/case_transform/src/lib.rs

+use ruru::result::Error as RuruError;

-class!(CaseTransform);
+trait Transform: Object {


Transform describes an interface for transformation. It can be implemented only to Ruby objects due to Object constraint.

Then we make type-specific implementations for

AnyObject (no-op)

RString

Symbol

Hash

Array

by implementing transorm methods.

d-unsed · 2016-09-21T20:52:53Z

ext/case_transform/src/lib.rs

-    fn transformSymbol(value: Symbol, transformMethod: &Fn(AnyObject) -> AnyObject) -> Symbol {
-        let transformed = transformMethod(value);
-        Symbol::new(transformed);
+trait TryTransform: Object {


TryTransform trait itself is needed only to define try_transform method on AnyObject. Rust does not allow to change the implementation of types that is not owned by current crate.

d-unsed · 2016-09-21T20:57:14Z

ext/case_transform/src/lib.rs

-        let transformed = transformMethod(value);
-        Symbol::new(transformed);
+trait TryTransform: Object {
+    fn try_transform<T>(&self,


try_transform<T> method:

Tries to convert current AnyObject to the type T.

:

If the conversion is successful it performs a transformation.

otherwise, returns an error from try_convert_to.

An example of usage of try_transform is

object.try_transform::<Hash>(key_transform) // if the object is a hash, try to do a hash transformation

The interesting part here is the constraints for T type: where T: VerifiedObject + Transform.

d-unsed · 2016-09-21T21:01:57Z

ext/case_transform/src/lib.rs

-            ValueType::Object => value
-        }
+fn transform(object: AnyObject, key_transform: &Fn(String) -> String) -> AnyObject {
+    let result = object.try_transform::<RString>(key_transform)


Try to transform the object as a RString
Otherwise try to transform the object as a Symbol
...
Otherwise try to transform the object as a AnyObject (it is any unkown class, do nothing)

d-unsed · 2016-09-21T21:03:12Z

ext/case_transform/src/lib.rs

-    fn toCamelCase(key: String) -> String { to_camel_case(to_snake_case(key.unwrap())) }
-    fn toDashedCase(key: String) -> String { to_kebab_case(to_snake_case(key.unwrap())) }
-    fn toSnakeCase(key: String) -> String { to_snake_case(key.unwrap()) }
+    result.unwrap()


We can safely unwrap here, because result has AnyObject type

- Add `Transform` and `TryTransform` traits - Move Rust functions outside `methods!` macro - Change naming to match conventions

d-unsed · 2016-09-21T21:08:00Z

ext/case_transform/src/lib.rs

-    fn dash(value: AnyObject) -> AnyObject { transform(value.unwrap().to_any_object(), &dash, &toDashedCase) }
-    fn underscore(value: AnyObject) -> AnyObject { transform(value.unwrap(), &underscore, &toSnakeCase) }
-    fn unaltered(value: AnyObject) -> AnyObject { value.unwrap().to_any_object() }
+    fn camel(object: AnyObject) -> AnyObject { transform(value.unwrap(), &to_pascal_case) }


And we can safely unwrap in this methods, because objects are AnyObjects

d-unsed · 2016-09-21T21:10:14Z

ext/case_transform/src/lib.rs

-    fn toSnakeCase(key: String) -> String { to_snake_case(key.unwrap()) }
+class!(CaseTransform);
+
+methods! (


All the functions which are not used directly in the CaseTransform class are moved outside methods!

this makes sense!

d-unsed · 2016-09-21T21:10:56Z

That's what I meant in d-unsed/ruru#35 :)

I hope it helps!

# Running:

....

Finished in 2.039933s, 1.9608 runs/s, 28.9225 assertions/s.

NullVoxPopuli · 2016-09-21T21:19:15Z

ext/case_transform/src/lib.rs

-
-use ruru::{Class, Object, RString, Hash, Array, Symbol, AnyObject, VM};
-use ruru::types::ValueType;
+use inflector::cases::{camelcase, classcase, kebabcase, snakecase};


this is handy!

is there a way to import the methods into the 'global' namespace of the file? so you don't have to do classcase::to_class_case?

NullVoxPopuli · 2016-09-21T21:19:54Z

ext/case_transform/src/lib.rs

-use ruru::types::ValueType;
+use inflector::cases::{camelcase, classcase, kebabcase, snakecase};

-class!(CaseTransform);


why was this removed? I saw it one one of the example repos, though I don't know what its purpose was

The line was moved to the bottom of the file

NullVoxPopuli · 2016-09-21T21:21:43Z

ext/case_transform/src/lib.rs

-methods! (
-    CaseTransform,
-    itself,
+trait Transform: Object {


I need to read up on traits and impls

for myself:

https://doc.rust-lang.org/book/traits.html

https://doc.rust-lang.org/book/generics.html looks like impl's are just implementations of a generic struct?

NullVoxPopuli · 2016-09-21T21:23:50Z

ext/case_transform/src/lib.rs

-    fn transformSymbol(value: Symbol, transformMethod: &Fn(AnyObject) -> AnyObject) -> Symbol {
-        let transformed = transformMethod(value);
-        Symbol::new(transformed);
+        result.into_iter()


arrays are iterable by default?

edit: not*

They kinda are iterable, but after explicit conversion to iterators :) Same for slices, vectors etc

NullVoxPopuli · 2016-09-21T21:27:33Z

ext/case_transform/src/lib.rs

-    fn toCamelCase(key: String) -> String { to_camel_case(to_snake_case(key.unwrap())) }
-    fn toDashedCase(key: String) -> String { to_kebab_case(to_snake_case(key.unwrap())) }
-    fn toSnakeCase(key: String) -> String { to_snake_case(key.unwrap()) }
+class!(CaseTransform);


here it is!

NullVoxPopuli · 2016-09-21T21:34:11Z

@d-unseductable this helps a ton! thanks so much!

NullVoxPopuli · 2016-09-21T21:34:42Z

I have to wait till I get to my personal laptop to try this out, so I have consistent hardware for my benchmarking :-)

d-unsed · 2016-09-21T21:47:02Z

You're welcome! Please let me know when you get the results

NullVoxPopuli · 2016-09-21T21:48:52Z

Just for some background here is the comparison between pure ruby, and my last successful compile: rails-api/active_model_serializers#1928 (comment)

It actually significantly hurt performance. haha.

NullVoxPopuli · 2016-09-21T22:58:17Z

rails-api/active_model_serializers#1928 (comment)

So, slightly faster... but still am not sure what the deal is with the slowness. Still, I appreciate you teaching me rust!

NullVoxPopuli · 2016-09-21T23:34:22Z

I guess what could be next is seeing if fiddle is slowing me down.

d-unsed force-pushed the transform_trait branch from 803b258 to 86b302e Compare September 21, 2016 20:33

d-unsed commented Sep 21, 2016

View reviewed changes

Refactor native extension

f7dc641

- Add `Transform` and `TryTransform` traits - Move Rust functions outside `methods!` macro - Change naming to match conventions

d-unsed force-pushed the transform_trait branch from 86b302e to f7dc641 Compare September 21, 2016 21:05

d-unsed commented Sep 21, 2016

View reviewed changes

NullVoxPopuli reviewed Sep 21, 2016

View reviewed changes

NullVoxPopuli merged commit 8056028 into rails-api:ruru-rust-speed-boost Sep 21, 2016

d-unsed deleted the transform_trait branch September 22, 2016 06:26

Conversation

d-unsed commented Sep 21, 2016

Uh oh!

d-unsed Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-unsed commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NullVoxPopuli Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-unsed Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NullVoxPopuli commented Sep 21, 2016

Uh oh!

NullVoxPopuli commented Sep 21, 2016

Uh oh!

d-unsed commented Sep 21, 2016

Uh oh!

NullVoxPopuli commented Sep 21, 2016

Uh oh!

NullVoxPopuli commented Sep 21, 2016

Uh oh!

NullVoxPopuli commented Sep 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

d-unsed Sep 21, 2016 •

edited

Loading

d-unsed commented Sep 21, 2016 •

edited

Loading

NullVoxPopuli Sep 21, 2016 •

edited

Loading

d-unsed Sep 21, 2016 •

edited

Loading