raze icon indicating copy to clipboard operation
raze copied to clipboard

Implement Raze::State for stronger storage typing

Open z64 opened this issue 7 years ago • 2 comments

A "proof of concept" for what my rambling #9 would look like.

Notes:

  • I maintain the basic hash-like interface, so this should not be a breaking change for anyone that doesn't use add_storage_context_type.
  • I add some basic properties to State that reflect the basic type union, so you can use those instead and only have to deal with T | Nil instead of Nil | String | Int32 | Int64 | Float64 | Bool
  • I see that params used the same type pool, StoreTypes, that is expanded by the original add_storage_context_type macro. However, I'm unsure what this means in terms of HTTP params.. I kept the basic type union for this for now. We could still do this by renaming my Raze::State to Raze::Storage and have:
getter state = Raze::Storage.new
getter params = Raze::Storage.new
  • Unrelated to this PR (kind of): I might note that using ctx.params=, someone might pass custom objects to URI.unescape(val) because the type would support it - I don't think this works as stdlib defines URI.unescape(string : String..). parameters should probably be restricted to Hash(String, String) I think for better debugging. Or maybe just URI.unescape(val.to_s).

I also saw your note, if I recall, to move away from global macros like get etc. and have Raze.get- this is a :+1: from me and I reflect this TODO here, but I'll move it back to global if you would like.

I might alternatively suggest Raze.state_property instead of add_..; that's just me though.

z64 avatar Oct 30 '17 17:10 z64

What's the state of this PR? Any thoughts @samueleaton

unreadable avatar Nov 02 '17 21:11 unreadable

@unreadable Sam just dropped a comment on my issue #9 the other day, I don't think he's gotten around to reviewing this yet. If you'd like to review it / respond to any of my points, please do :) I don't know that many other users of Raze so it would be good to get more opinions on this.

z64 avatar Nov 02 '17 21:11 z64