added enum support

[confact]

Got help a bit from @paulcsmith with this.

It is a bit hacky.

Instead of using enum Status, you use this macro.

Like this:

class Issue <  BaseModel
avram_enum Status do
  Opened = 1
end

and for the int32 type add Status as type instead.

It shall now work, but with some changes in create and update:

status: Issue::Status.new(:opened)

No tests as I am not sure how you would like to test this macro?

[jkthorne]

I think you might want to add some specs for this.

[confact]

@wontruefree added two tests. Will add more later if that is not enough :)

update: adding a migration for my test broke another test. Will fix that tomorrow ^^

[jwoertink]

I love this! I have a few enums in my apps, so it'll be nice. I left a few questions here to see what else we can do to test this out.

[akadusei]

Since, it seems, the actual enum won't be used in the public API, why not use the Avram prefix for that instead?

So that one can do, for instance:

class Issue < BaseModel
  avram_enum Status do
    Inactive
    Active
  end

  table :issues do
    column status : Issue::Status # <= instead of `Issue::AvramStatus`
  end
end

I don't know, but defining a Foo enum and referencing it as AvramFoo seems a bit weird.

(Edit: I'm not familiar with how reviews work on GitHub, so forgive any mishaps. I'm open to any advise regarding this.)

[akadusei]

Suggested change

	updated_issue.status.value.should eq(Issue::Status::Closed)
	updated_issue.role.value.should eq(Issue::Role::Issue)
	updated_issue.status.value.should eq(Issue::AvramStatus::Closed)
	updated_issue.role.value.should eq(Issue::AvramRole::Issue)

[confact]

This won't work, sadly. As it can't access the constant on enum and such. I added a getter on @enum to get the constant.

[paulcsmith]

@akadusei i think that if that works that is a very nice api! @confact could you give that a try and see if it passes? I think that’d be a pretty cool API!

[confact]

@paulcsmith @akadusei this looked pretty neat. Only issue I found is that value wouldn't return constant, but the value of the enum, here it would be Int32 - So I added a getter to @enum variable to access it to read the constant.

not 100% sure about that, would do you guys think?

[akadusei]

@paulcsmith @akadusei this looked pretty neat. Only issue I found is that value wouldn't return constant, but the value of the enum, here it would be Int32 - So I added a getter to @enum variable to access it to read the constant.

not 100% sure about that, would do you guys think?

I think it's great. Having a getter for the raw value in a wrapper makes a lot of sense. This way, one does not ever need to access the actual enum directly. Maybe we should mark the actual enum private?

I would make the wrapper a struct instead of a class because enums are value types, and structs, in general, make sense for immutable type wrappers. This is not required though, so maybe a class is OK.

[paulcsmith]

Thank you so much @akadusei and @confact

I love this new approach and I think it’s all quite clean and intuitive. I wasn’t sure about the lat comment you left though @confact. Could you add a spec to https://github.com/luckyframework/avram/blob/master/spec/save_operation_spec.cr that tests creating and updating? Maybe that’ll help show what the issue is?

[paulcsmith]

Could this be Issue::Role::Issue? This is totally fine as is Just curious if it is possible to do that with boxes

[confact]

Sadly no, as the constants are inside the enum, this is the wrapper. I haven't found a way to directly use enums.

[jwoertink]

Would Issue::AvramRole::Issue work?

[jwoertink]

Thinking about this, I don't think it would work. It would end up throwing an undefined method parse for that enum constant. I think my only hangup was that I wasn't too keen on the interface; however, I do see why it's like this.

We can probably just roll with this as is, and always change it up later if we come up with a new idea. Like Issue::Status[:closed] for example... This is a pretty decent start and definitely provides a lot more than what we have now. 👍

[jwoertink]

Thanks for tackling this! I know I'll be needing to use it for sure. If @paulcsmith is cool with this, then we can merge in when ready.

[confact]

@jwoertink I am not 100% keen with this either. But it is the best way to do it for now. As soon we have updated the type extensions not to be extensions I am sure we can do this in a better way.

For now, this is the only way to do it.

[paulcsmith]

@confact Makes sense! It is still quite nice IMO and I'm excited to merge this. Thanks @confact and @akadusei!