search

yetibot / core

:expressionless: Core yetibot utilities, extracted for shared use among Yetibot and its various plugins
https://yetibot.com
Eclipse Public License 1.0
27 stars 17 forks source link

Switch from schema to clojure.spec #106

Closed anthonygalea closed 5 years ago

anthonygalea commented 5 years ago

This is a first stab at migrating to clojure.spec. I wouldn't call it ready to be merged but thought it would be a good idea to get feedback early.

devth commented 5 years ago

Whoa, this is awesome! I'll review in detail when I get a chance - hopefully later today. But at first glance looks like the right approach.

devth commented 5 years ago

@anthonygalea looks good so far! I noticed you renamed yetibot.core.adapters.init to yetibot.core.adapters. I'm fine w/ it but what's the intention? Thanks!

anthonygalea commented 5 years ago

This is (I think) a minor improvement in naming with the aim of making things more readable. ::config being a namespaced keyword is :adapters/config instead of :adapters-init/config. This is both shorter and in line with all the other "configs" ex: :logging/config. We don't have :logging-init/config even though it is also used for initialization. In addition here we could have adapters/config instead of ai/adapters-config.

I typically like to work such changes piecemeal into commits as I encounter them. Having said that if such changes bug you let me know and I'll refrain from doing them.

anthonygalea commented 5 years ago

Reviewed pull request, made additional changes and rebased on latest master. Also reviewed corresponding pull request in yetibot.

devth commented 5 years ago

Thanks for the explanation! I agree - :adapters/config is much nicer. And I'm fine with changes like this - just wanted to understand the intent.

I'm trying to figure out why the build is failing. It looks like the db schemas are empty at https://travis-ci.com/yetibot/yetibot.core/jobs/230115009#L250, so the database isn't being properly setup for test cases, which causes subsequent failures.

anthonygalea commented 5 years ago

Fixed, it was a bug in the spec for :schema/spec.

anthonygalea commented 5 years ago

Thanks a lot for your feedback @devth! Just addressed the comments.

Regarding spec2, thanks a lot for the link explaining the differences. I would prefer to tackle this in a future pull request if it's ok with you.

devth commented 5 years ago

Regarding spec2, thanks a lot for the link explaining the differences. I would prefer to tackle this in a future pull request if it's ok with you.

Yep, sounds good!

anthonygalea commented 5 years ago

Great @devth. Yep good to merge.