Skip to content

Further update Granite to be compatible with Crystal 0.25#231

Merged
robacarp merged 7 commits into
masterfrom
granite_new_fix
Jun 19, 2018
Merged

Further update Granite to be compatible with Crystal 0.25#231
robacarp merged 7 commits into
masterfrom
granite_new_fix

Conversation

@robacarp
Copy link
Copy Markdown
Member

The latest granite tag produces this when attempting to build amber projects which used the scaffold generator:

instantiating 'PostCommentController#create()'
in src/controllers/post_comment_controller.cr:22: no overload matches 'PostComment.new' with type Hash(String, String | Nil)
Overloads are:
 - Granite::Base.new(args : Hash(Symbol | String, String | JSON::Any))
 - Granite::Base.new(**args : Object)
 - Granite::Base.new()

    post_comment = PostComment.new(post_comment_params.validate!)

In trying to get the amber project compatible with crystal 0.25, I'd like to change this to JSON::Any::Type, which allows the generated code to compile.

@eliasjpr
Copy link
Copy Markdown
Contributor

I have asked in the Crystal channel about JSON::Any and JSON::Any::Type I think the later makes more sense to keep things compatible. Not sure waiting for a response from the Crystal channel

@eliasjpr
Copy link
Copy Markdown
Contributor

I think we should be using JSON::Any::Type everwhere we had JSON::Type since both points to a union type

@robacarp
Copy link
Copy Markdown
Member Author

I’d discourage tagging after this is merged until it’s known that no other changes need to be made to Granite to make amber compatible

@eliasjpr
Copy link
Copy Markdown
Contributor

To be safe we should probably use def initialize(args : Hash(Symbol | String, JSON::Any | JSON::Any::Type)) this will support the new way and the previous way

Comment thread src/granite/base.cr Outdated
end

def initialize(args : Hash(Symbol | String, String | JSON::Any))
def initialize(args : Hash(Symbol | String, JSON::Any::Type))
Copy link
Copy Markdown
Contributor

@eliasjpr eliasjpr Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets define a initialize overload, I think it would be better

def initialize(args : Hash(Symbol | String, JSON::Any::Type))
  new(JSON::Any.new raw(args))
end

def initialize(args : Hash(Symbol | String, JSON::Any))
end

Comment thread src/granite/base.cr Outdated
set_attributes(args)
end

def initialize(args : Hash(Symbol | String, Granite::Fields::Type))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread src/adapter/mysql.cr
stmt << "("
stmt << Array.new(fields.size, '?').join(',')
params.concat fields.map { |field| model.to_h[field] }
params.concat fields.map { |field| model.read_attribute field }
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat, i like that better.

@robacarp robacarp changed the title Add string? to granite::base#new Further update Granite to be compatible with Crystal 0.25 Jun 18, 2018
robacarp added 3 commits June 18, 2018 15:45
- New libraries handle enforcing UTC internally
- UTC in adapters is rounded to the second, so timestamps must be
  .at_beginning_of_second
@eliasjpr
Copy link
Copy Markdown
Contributor

👍🏻

@eliasjpr
Copy link
Copy Markdown
Contributor

Sweet this is now passing

@robacarp robacarp merged commit 80352ea into master Jun 19, 2018
@robacarp robacarp deleted the granite_new_fix branch June 19, 2018 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants