Amber: Amber with Crecto does not compile

[KevinSjoberg]

Description

Creating a new Amber application using the Crecto database wrapper does not compile when running amber watch.

Steps to Reproduce

1. amber new blog -d pg -t slang -m crecto
2. cd blog
3. amber watch

Expected behavior: [What you expect to happen]

The application compiles successfully.

Actual behavior: [What actually happens]

It does not compile due to a type mismatch issue.

instance variable '@uri' of Crecto::Repo::Config must be String, not (String | Nil)

In Crecto, uri is defined as String. In src/amber/environment/settings.cr#L15 we define database_url as String as expected.

On the other hand, our YAML mapping macro does not. In src/amber/environment/settings.cr#L40 we define database_url as String? with a default value of nil.

In our newly generated blog, we use this value as the Crecto uri.

blog/config/initializers/database.cr

require "pg"
require "crecto"
Query = Crecto::Repo::Query

module Repo
  extend Crecto::Repo

  config do |c|
    c.adapter = Crecto::Adapters::Postgres

    c.uri = Amber.settings.database_url
  end
end

Crecto::DbLogger.set_handler(Amber.settings.logger)

Reproduces how often: [What percentage of the time does it reproduce?]

100% of the time.

Versions

Amber:

Amber CLI (amberframework.org) - v0.6.4

Crystal:

Crystal 0.24.1 (2017-12-26)

LLVM: 5.0.1
Default target: x86_64-apple-macosx

Crecto:

  crecto:
    github: Crecto/crecto
    version: 0.8.3

OS:

MacOS High Sierra
Version 10.13.2 (17C88)

I'd be happy to submit a PR as we decide what the best approach is. If we expected database_url to always be set, I'd just set the type to String.

[eliasjpr]

@KevinSjoberg thank you for submitting this issue.

We allow the database_url setting to be nil given the fact that there could be projects that do not need a database url.

Can you try c.uri = Amber.settings.database_url.not_nil! for the moment.

I would like to invite @amberframework/contributors and @amberframework/core-team to jump in this conversation to come to a consensus on the best approach

[faustinoaq]

@KevinSjoberg @eliasjpr You can use c.uri = Amber.settings.database_url.to_s as well

[KevinSjoberg]

@eliasjpr @faustinoaq I'm aware of how we can solve it. My point is that the user experience isn't ideal. Do we want to update the template for database.cr to include the workaround?

I'd be happy to help out in any way possible. 🙂

[eliasjpr]

@KevinSjoberg yes please, and thank you.

Please do not use the to_s as per recommendation of Crystal

In many programming languages what will happen is that to_s, or a similar method for converting the object to its string representation, will be invoked, and then that string will be written to the standard output. This works, but it has a flaw: it creates an intermediate string, in heap memory, only to write it and then discard it. This, involves a heap memory allocation and gives a bit of work to the GC.

[KevinSjoberg]

Thinking further, do we want to ensure that the type for database_url is matched for what we expect from the YAML macro? Currently, Amber defines the database_url property as String but the YAML mapping macro as String?. If database_url can be nil, we probably should say that for the property as well?

[KevinSjoberg]

After reading some of the comments in Gitter, let's default to an empty string.

[faustinoaq]

In many programming languages what will happen is that to_s, or a similar method for converting the object to its string representation, will be invoked, and then that string will be written to the standard output. This works, but it has a flaw: it creates an intermediate string, in heap memory, only to write it and then discard it. This, involves a heap memory allocation and gives a bit of work to the GC.

@eliasjpr Thank you for your comment! I didn't know that, Maybe we should change this, this and this

[eliasjpr]

@KevinSjoberg are we good with this issue? Can we close it?

[marksiemers]

@eliasjpr and @faustinoaq

Regarding:

Please do not use the to_s as per recommendation of Crystal

Assuming that is coming from here, that is specific to writing to an IO (usually stdout):

Don't create intermediate strings when writing to an IO

If this doesn't write to an IO, then using to_s or another way to get a string shouldn't matter.

[KevinSjoberg]

@eliasjpr yes, closing. As soon as Crecto publishes a new version everything should be ok.