GenieFramework / SearchLight.jl

ORM layer for Genie.jl, the highly productive Julia web framework
https://genieframework.com
MIT License
137 stars 16 forks source link

DateParser is not a dependency of SearchLight #8

Closed lucianolorenti closed 5 years ago

lucianolorenti commented 5 years ago

I tried to add it but I the package manager forced me to update a bunch of packages and i don't know if that is desired. That is why I did not make a PR.

And also, DateParser is not being imported in SearchLight.jl

essenciary commented 5 years ago

Thanks - I'll add it

Benardi commented 5 years ago

I've ran into this problem very recently. I'm trying to provide a Date as a parameter as I try to seed a SQL table. However, SearchLight demands DateParser:

ERROR: UndefVarError: DateParser not defined Stacktrace: [1] convert(::Type{Dates.Date}, ::String) at /home/benardi/.julia/packages/SearchLight/AjZuW/src/SearchLight.jl:3787 [2] #TestExecution#1 at /home/benardi/workspace/ts_insights/app/resources/testexecutions/TestExecutions.jl:65 [inlined] [3] Type at ./none:0 [inlined] [4] seed() at /home/benardi/workspace/ts_insights/app/resources/testexecutions/TestExecutions.jl:74 [5] top-level scope at none:0

But even if I add DataParser to my app (instantiate and build it) SearchLight stumbles into the same error.

essenciary commented 5 years ago

Thanks - DateParser seems to be defunct, hasn't been updated in quite a while and it doesn't support Julia 1.x. If I remember correctly that stopped me from adding it.

Did you manage to add it to your project? I'm getting

(SearchLight) pkg> add DateParser
 Resolving package versions...
ERROR: Unsatisfiable requirements detected for package DateParser [c513aa4d]:
 DateParser [c513aa4d] log:
 ├─possible versions are: 0.1.0-0.1.1 or uninstalled
 ├─restricted to versions * by an explicit requirement, leaving only versions 0.1.0-0.1.1
 └─restricted by julia compatibility requirements to versions: uninstalled — no versions left
essenciary commented 5 years ago

I'm thinking about just removing these conversion methods - and let the user handle the conversions, since the new Dates API requires a date format anyway, which should be provided by the user.

essenciary commented 5 years ago

I've been running some tests and it looks like a lot of the conversion logic no longer works in Julia 1. I need to take a closer look.

Benardi commented 5 years ago

The package DateParser builds fine but fails to pre-compile most likely for the reasons you mentioned. What then would be the procedure to save an object in a table that contains a column of type Date using SearchLight?

essenciary commented 5 years ago

@Benardi @lucianolorenti I rewrote the converters logic. I removed DateParser as a dependency as it wasn't updated recently and didn't work on Julia v1.

I added a new API which allows injecting converters as needed. To add a date converter you can use this:

SearchLight.@converter convert(::Type{Date}, x::String) = Date(x)

Basically, you pass the convert function you want to the @converter macro.

Don't forget to do pkg> up to get the latest SearchLight.

Benardi commented 5 years ago

@Benardi @lucianolorenti I rewrote the converters logic. I removed DateParser as a dependency as it wasn't updated recently and didn't work on Julia v1.

I added a new API which allows injecting converters as needed. To add a date converter you can use this:

SearchLight.@converter convert(::Type{Date}, x::String) = Date(x)

Basically, you pass the convert function you want to the @converter macro.

Don't forget to do pkg> up to get the latest SearchLight.

What would be the best way to use such converter injections in a Genie.jl app? As far I could see/test changes like this are tied to the current environment. In terms of SearchLight + Genie this injection seems to truly work when it's done after the app has been loaded. An injection like this needs to be easily applied multiple times, for instance each time a Genie App starts up. Much like node applications that use NPM Genie.jl packages are somewhat ephemeral especially when we think about deployment methods that use containers.

(Perhaps this is a Genie.jl issue)

@essenciary Can you think of or propose a good way to pragmatically make use of this feature in a Genie.jl app ?

essenciary commented 5 years ago

I think this can be safely closed now. The API has changed a lot and the standard way is to add specialized methods to Base.convert. The place for adding these is config/initializers/converters.jl for Genie apps.