Skip to content
This repository has been archived by the owner on Oct 28, 2019. It is now read-only.

Corrects macro that generates create method of forms #312

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Corrects macro that generates create method of forms #312

wants to merge 1 commit into from

Conversation

Xosmond
Copy link
Contributor

@Xosmond Xosmond commented Jan 22, 2019

Fixes #311

@edwardloveall
Copy link
Member

Hey @Xosmond! We've moved and renamed LuckyRecord to Avram. We decided to move it instead of using GitHub's rename option so that current code that relies on lucky_record would continue to work and a project must opt-in to the new shard. However, a big bummer is we can't move PRs.

If you are still interested in this change, would you be able to rebase onto Avram? If not don't worry about it. We can eventually rebase it for you. Thanks 😄! Let us know if you have any questions.

@Xosmond
Copy link
Contributor Author

Xosmond commented Feb 3, 2019

@edwardloveall Hey sure, will do. But actually I wanted to know your opinion and also @paulcsmith about two ways for creating records.

This PR would fix the bug but also allow to create records like this (having username is a regular field and password andpassword_confirmation are virtual fields):

RegistrationForm.create! username: "hello", password: "my password", password_confirmation: "my password"

But only fixing virtual parameters bug, would need us to write it like this:

RegistrationForm.create! params: { "username" => "hello" }, password: "my password", password_confirmation: "my password"

So which do you think would be better?

@paulcsmith
Copy link
Member

I love that first version. Looks more type safe and I think it’s more intuitive! 👍

@Xosmond
Copy link
Contributor Author

Xosmond commented Feb 4, 2019

@paulcsmith Ok i will move this and create the tests but I see NEEDS_ON_INITIALIZE, NEEDS_ON_CREATE and NEEDS_ON_UPDATE are not actually used, do you have a reason for these constants? I ask this because they can generate possible bugs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants