activegraph icon indicating copy to clipboard operation
activegraph copied to clipboard

Add TypeConverter DSL

Open jgaskins opened this issue 6 years ago • 4 comments

This pull introduces/changes:

  • Added Neo4j::Shared::TypeConverters.create DSL method to create a simple converter

I noticed that most of the converters are written as classes with all their methods defined on self and they could just be object instances with the primitive_type and convert_type being stored as instance variables rather than full-on methods. And since most of the type converters I write are simple conversions, I extracted most of my converters to look like this:

Neo4j::Shared::TypeConverters.create db: String, ruby: BCrypt::Password do
  db(&:to_s)
  ruby do |value|
    if BCrypt::Password.valid_hash?(value)
      BCrypt::Password.new(value)
    else
      BCrypt::Password.create(value)
    end
  end
end

In my own app, this is all within the Conversion class, which is why it's still called that in this PR — I straight-up pasted from my own code and extracted the method to live on the TypeConverters module. I wasn't sure if this was something people might want, so I haven't cleaned it up much, but I wanted to offer it up in case anyone found it useful.

jgaskins avatar Feb 20 '18 13:02 jgaskins

Codecov Report

Merging #1484 into master will decrease coverage by <.01%. The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1484      +/-   ##
==========================================
- Coverage   96.85%   96.85%   -0.01%     
==========================================
  Files         205      205              
  Lines       12615    12642      +27     
==========================================
+ Hits        12218    12244      +26     
- Misses        397      398       +1
Impacted Files Coverage Δ
...ntegration/type_converters/type_converters_spec.rb 100% <100%> (ø) :arrow_up:
lib/neo4j/shared/type_converters.rb 95.65% <93.75%> (-0.15%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 961e28c...940a9d7. Read the comment docs.

codecov-io avatar Feb 20 '18 14:02 codecov-io

This is awesome, thank you! A couple of things:

  • Could you change the documentation to reflect this DSL? I think that this would be the appropriate section
  • For an example, I didn't think about the fact that db(&:to_s) was equivalent to db {|value| value.to_s }, leading me to think that the whole block was uneven (I thought that you returned the DB value and defined a ruby block). It might be good in the docs to use a explicit block for whatever example it is to avoid confusion and people can optimize as they like

cheerfulstoic avatar Feb 23 '18 01:02 cheerfulstoic

Could you change the documentation to reflect this DSL?

Heck yes I can.

Speaking of docs, this project has had great docs ever since I first looked at it, but they've improved a lot since 8.0 and I really appreciate your continued efforts on that front.

I didn't think about the fact that db(&:to_s) was equivalent to db {|value| value.to_s }

I actually meant to change that here for that exact reason and forgot before posting. The shorthand is not very expressive in this situation at all, but easy to write. My coworkers always call me out on stuff like that, too. My bad. :-)

jgaskins avatar Feb 23 '18 01:02 jgaskins

Thanks on all counts ;) I try to operate under the assumption that is somebody has a question then the docs fell short, but there are still some holes I'd like to fill eventually ;)

And I totally do stuff like db(&:to_s) myself and am fine with it, I just think the other way would be more accessible (though I don't think I need to explain this, given your comments ;) )

cheerfulstoic avatar Feb 23 '18 01:02 cheerfulstoic