Skip to content

Update to crystal 0.26#922

Closed
bcardiff wants to merge 1 commit intoamberframework:masterfrom
bcardiff:crystal/0.26.0
Closed

Update to crystal 0.26#922
bcardiff wants to merge 1 commit intoamberframework:masterfrom
bcardiff:crystal/0.26.0

Conversation

@bcardiff
Copy link
Copy Markdown
Contributor

@bcardiff bcardiff commented Aug 7, 2018

Note: This changes should be backward compatible with 0.25.1
Ref: crystal-lang/crystal#6414

ivar initializers will be evaluated without self in context.

Disclaimer: I've only checked that the source build with crystal nightly together with this patch.

@filename : String
@filelogs : String

def initialize(__previous, __argv)
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.

Why not use *args and super?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was the minimum change. But it should be equivalent.

@Sija
Copy link
Copy Markdown
Contributor

Sija commented Aug 8, 2018

Also, there's one more:

Error in src/pet-tracker.cr:3: instantiating 'Amber::Server.class#start()'

Amber::Server.start
              ^~~~~

in lib/amber/src/amber/server/server.cr:17: instantiating 'Amber::Server#run()'

      instance.run
               ^~~

in lib/amber/src/amber/server/server.cr:50: instantiating 'start()'

        start
        ^~~~~

in lib/amber/src/amber/server/server.cr:59: undefined method 'tls=' for HTTP::Server

      server.tls = Amber::SSL.new(settings.ssl_key_file.not_nil!, settings.ssl_cert_file.not_nil!).generate_tls if ssl_enabled?
             ^~~

@bcardiff
Copy link
Copy Markdown
Contributor Author

bcardiff commented Aug 8, 2018

I didn't detect that because building the specs does not stress that path 🤔 .

To fix that, either something like kemalcr/kemal#477 can be done, or
since crystal-lang/crystal#6500 is merged, a more opaque bind can be used.

@bcardiff
Copy link
Copy Markdown
Contributor Author

bcardiff commented Aug 9, 2018

I also noticed that the shell-table shard version will need to be updated to support 0.26.0 jwaldrip/shell-table.cr#2

@robacarp
Copy link
Copy Markdown
Member

@bcardiff thanks for this, I've picked your commit over to #929 and will finish the effort there.

@robacarp robacarp closed this Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants