Look in to moving away from monkey-patched core types

[jwoertink]

We patch all of the basic types that Avram supports like this:

avram/src/avram/charms/bool_extensions.cr

Lines 1 to 6 in 6922463

	struct Bool
	def self.adapter
	Lucky
	end
	module Lucky

This is used in a lot of different places like

avram/src/avram/criteria.cr

Line 121 in 6922463

add_clause(Avram::Where::GreaterThanOrEqualTo.new(column, V.adapter.to_db!(value)))

avram/src/avram/add_column_attributes.cr

Line 66 in 6922463

parse_result = {{ attribute[:type] }}.adapter.parse({{ attribute[:value] }})

avram/src/avram/base_query_template.cr

Line 13 in 6922463

\{{ type }}.adapter.criteria(self, %("#{table_name}"."\{{ name }}"))

The idea is that your column specifies a type like admin : Bool, then we call Bool.adapter.whatever internally. I believe the original intention was to have a swappable adapter to handle doing different things, but we never got around to that.

The other bit is this ensures that Bool related methods are handled by Bool and String related methods are handled by String which gives Lucky (and Avram) an extra layer of type-safety without having to fight the compiler.

There are some downsides to this, and the most recent is that the new crystal tool unreachable doesn't work with Lucky at all, and it may boil down to this.
Another issue is that it makes creating custom types a lot more involved.

I think we can come up with a better interface for this which will also alleviate monkey-patching the stdlib types

[straight-shoota]

Just want to note that the level of monkey-patching is very low. As far as I see, it's just adding the self.adapter method, right? This could of course cause issues if someone else defines the same method, but otherwise should be fine. Adding an enclosed namespace with the shard name (Lucky) seems also pretty legit. There should be no relevant cause for collision.
So the only external argument would be working around a bug in crystal tool unreachable. But this should really be fixed in the compiler.

I can't say anything about the internal motivations for changing this.

[Blacksmoke16]

Yea came here to say looks like this will be resolved via crystal-lang/crystal#15065. So at this point sounds like the only downside is custom type support being more involved.

[jwoertink]

Thanks for chiming in and letting me know. That does make me feel better about holding off on this. I'd much rather save this level of change for a 2.0 than have to do things sooner, so that's good news. 🙌

[robacarp]

I'm in general very weary of monkeypatching, but I think here it presents a meaningful and helpful interface. I would back switching the patch to call the method lucky_adapter or avram_adapter to make it obvious where the method is coming from (and to reduce collisions), but there's no real reason it needs to happen soon -- at this point avram is most likely to be the first mover in any codebase and anyone finding a collision is going to be fixing that on their end.