Better check for structs

[JKRT]

If a user names a struct in all lowercase we will throw an error, this should maybe check that the type is a struct or abstract struct instead

[ghost]

Haha yeah thanks I think this is a good suggestion! The existing check is kind of silly!

I think the reason that this PR and your other PR https://github.com/RelationalAI-oss/Rematch.jl/pull/20 failed is because you're trying to check information about the runtime type T, but here inside the macro, T is just an Expression or a Symbol.

What I think this should do is add the assertion you wrote into the generated code produced by the macro. Glancing through the code, I think the way you do that is by pushing an Expr into the asserts array, as is done here: https://github.com/JKRT/Rematch.jl/blob/76dfc7a25f6b98e3bfb2dac4199b68876ae9d7a8/src/Rematch.jl#L142

So in your case, i think that would be something like:

            push!(asserts, quote
                if !(isstructtype($T) || issabstracttype($T))
                    throw(AssertionError(...))
                end
            end)

[JKRT]

Haha yeah thanks I think this is a good suggestion! The existing check is kind of silly!

I think the reason that this PR and your other PR #20 failed is because you're trying to check information about the runtime type T, but here inside the macro, T is just an Expression or a Symbol.

What I think this should do is add the assertion you wrote into the generated code produced by the macro. Glancing through the code, I think the way you do that is by pushing an Expr into the asserts array, as is done here: https://github.com/JKRT/Rematch.jl/blob/76dfc7a25f6b98e3bfb2dac4199b68876ae9d7a8/src/Rematch.jl#L142

So in your case, i think that would be something like:

            push!(asserts, quote
                if !(isstructtype($T) || issabstracttype($T))
                    throw(AssertionError(...))
                end
            end)

@rai-nhdaly Great that we are in agreement 💯 I was just doing a quick and dirty fix hoping to get away with it. Sadly I did not :(, However, the above might not work. We should also check that it is not a function, Julia will report that functions is struct if I am not mistaken. So something like:

if ($T <: Function) || !(isstructtype($T) || issabstracttype($T)) Would that suffice?

[JKRT]

@rai-nhdaly I updated it per your suggestions. It will still fail for later versions of Julia since the stuff I use is not really backward compatible. So compat needs to be changed for the next release

(Edit) seems that I broke the 0.6 build Might it be time to make it obsolete on maser considering Julia 1.2 is released soon?

[JKRT]

@rai-nhdaly Since you removed support for Julia 0.6 in #22 Maybe we can trigger the build again if you like my proposed changes

[ghost]

I do, thanks. Can you merge master into your branch and then re-push? Thanks for your patience with this PR! :)

[JKRT]

Hi,

Yes but I cannot promise that I have time to do this today. Maybe later tonight (I am in Sweden) : )

[ghost]

:) No problem. Take your time!

On Tue, Sep 17, 2019 at 12:44 PM John notifications@github.com wrote:

Hi,

Yes but I cannot promise that I have time to do this today. Maybe later tonight (I am in Sweden) : )

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RelationalAI-oss/Rematch.jl/pull/21?email_source=notifications&email_token=AKSS5LAVGISNHIZH7X5DTF3QKECNRA5CNFSM4IFTRGO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD65E2WA#issuecomment-532303192, or mute the thread https://github.com/notifications/unsubscribe-auth/AKSS5LGSTLC6PZ77NORQYUTQKECNRANCNFSM4IFTRGOQ .

[JKRT]

@rai-nhdaly I have rebased. Please restart my build 💯

[ghost]

I think you need to push up your rebased branch to github. (via git push --force).

It doesn't show any changes since July (sorry again for our delay 😅)

[JKRT]

@rai-nhdaly fixed :)

[ghost]

Awesome, thanks @JKRT!

I'm sorry this review got paused for a long time. :) Thanks for the contribution!