feat: TypeCheck integration #34
Conversation
|
@dvic as TypeCheck doesn't work with Elixir 1.9 I think the easiest we can do is to just stop testing with Elixir 1.9 as well. Our library should still work if you don't include TypeCheck, but Elixir 1.10 was released in Jan 2020, so I think we should be okay not testing for Elixir 1.9 now. |
| quote bind_quoted: [types: types] do | ||
| @type t() :: %__MODULE__{unquote_splicing(types)} | ||
| end | ||
| use_typecheck? = TypeCheck.Macros in __CALLER__.requires |
There was a problem hiding this comment.
Does this also work when you don't directly have use TypeCheck? e.g., use MyOwnMacro that generates the use TypeCheck part?
There was a problem hiding this comment.
Yes, because in order to have @type! working, TypeCheck.Macros have to be required.
There was a problem hiding this comment.
In fact, this will work even if you just do require TypeCheck
I think that's fair 👍. |
I think this is a nice solution! But I guess we should publish this as a breaking change? (major version bump)? Otherwise users might miss this big change in the changelog? |
I agree. Maybe we can start with an experimental feature config so we can test out and follow up with a breaking change major release. The only scenario it will break is if someone has something like that: defmodule MySchema do
use TypedEctoSchema
use TypeCheck
typed_embedded_schema do
field :int, :int
end
# This will break now because we will instruct TypeCheck to generate this function
def t, do: :ok
end |
|
Ok, gave some more though, I don't want to add config options, therefore I'll just release a pre release or a release candidate so we can test things out. We are still in pre-1.0, so technically a minor bump can introduce breaking changes. Would you release the 1.0 or just something like 0.5? |
|
Just found out that this doesn't work with non embedded schemas because of In fact, I just realized this will break with so many things because TypeCheck requires all types to be TypeCheck types. I'll hold this back a little bit and I think we will need a feature flag for that as it can break in many places. |
|
We will need the overrides for the following types:
|
|
@dvic added a feature flag config and docs so we can safely merge this and start working on the overrides. Maybe it is worth creating a library just for the overrides like |
|
@Qqwy what are your thoughts on this? What you would suggest? |
Cool! I'm ok with both options, but how would this work with the current approach? (i.e., detecting whether or not to use TypeCheck by checking the As an alternative we could also choose to have something like defmodule InteropWithTypeCheck do
use TypedEctoSchema.TypeCheck
use TypedEctoSchema
typed_embedded_schema do
field(:year, :number)
end
endto control this at module level, then we wouldn't need the experimental flag anymore as well as the |
|
@dvic My idea would be to just have another list of overwrites so we could do: defmodule InteropWithTypeCheck do
use TypeCheck, overrides: EctoTypeCheck.overrides()
use TypedEctoSchema
endThat being said, what could be an interesting proposal for typecheck would be to have modules that would return either a replacement or nil so people could configure like: config :type_check,
override_providers: [TypeCheck.Overrides, EctoTypeCheck]Because the current implementation requires either calling the config :type_check, overrides: [
{&Original.t/0, &Replacement.t/0}
]However, I think a simpler alternative would be to have our own types and change the type mapper to return this types directly instead of relying on type check override feature. I'll give that approach a try. |
|
The best (most flexbile while still being explicit and not overly boilerplate-y) pattern if you have a lot of overrides or other options to pass to TypeCheck is to have a single module, say This is similar to how e.g. |
|
I like the idea of checking whether The best thing to check for probably, is the existence of the TypeCheck.Options module attribute. The existence of this can be seen as a rigid piece of 'public API' which future versions of TypeCheck will not change 👍. |
|
Just to give an update on that for those waiting: I'm looking for a way to make it easier to generate the overrides for foreign libraries, so far I managed to create a script that automatically generates overrides for one file. As soon as I have something I'm happy with and I have all the required overrides for Ecto types I'll update you here. |
|
@dvic I think I have something worth looking now. It was actually easier than I thought. The code for sure can be improved and maybe we should think wether we want to include all these overrides as part of this library os as a different library. @Qqwy I'd love to know your opinion on this method of generating overrides automatically. It is pretty naive for now, but maybe it can be an interesting library for TypeCheck or even part of TypeCheck itself with some improvements. |
|
@dvic One problematic thing though is that the generated overrides are dependent on the specific Ecto version, so by extracting into other libraries (like |
bamorim
force-pushed
the
feat-type-check-integration
branch
from
7a90a6f
to
61a7a62
Compare
Nice! I gave it a quick glance (don't have the bandwidth to do a proper review this week) but it looks good to me!
Nice! One thing we could also do is support a limited set of Ecto versions and use |
|
@jtormey would you be willing to test this in your application to see how well it behaves? |
|
Absolutely! This is incredibly exciting and I'm amazed at how quickly you all pulled this together, very appreciated 🙂 |
|
I've started testing this out in our project and have hit a couple issues, dropping them here but will continue to see what I run into in the meantime.
Error details==> typed_ecto_schema Compiling 25 files (.ex)== Compilation error in file lib/typed_ecto_schema/overrides/ecto/association.ex ==
Sample code: defmodule Upside.TypesSchema do
use TypedEctoSchema.TypeCheck
use TypedEctoSchema
typed_schema "types", type_check: true do
field :my_field, :string
end
@spec! change(__MODULE__.t()) :: Ecto.Changeset.t()
def change(type) do
type
|> Ecto.Changeset.change()
end
endError detailsCompiling 1 file (.ex)== Compilation error in file lib/upside/types.ex == |
|
For the second issue, can you try changing the order of the use calls? |
|
Changing the order of the |
|
@jtormey thanks for reporting that, I'll check workarounds for that when I have some time. |
|
The first issue seems to be caused by the way the list of overrides is being built. It ends up being The second issue might be caused by somewhat of a technical limitation inside TypeCheck because of the way Elixir modules are compiled: in the module body (i.e. outside of the body of the functions) we do not have access to types that are compiled in the same module yet, so TypeCheck creates a separate 'internal' module that only contains all types, and compiles all usage of types in the module body using those. @bamorim you might want to try to use |
|
Absolutely amazing work in this PR by the way! 💚 |
closes #33
I decided to not even include an option as I feel options are a workaround for good experience.
My solution instead was to automatically check whether TypeCheck.Macros are required and just call it directly.
If you think it is too risky, we can add an option to enable that just as an "experimental feature" and we can warn people that this will change in the future (the feature will be enabled by default). What do you think?