Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Magic Login submodule #8

Closed
wants to merge 3 commits into from
Closed

Conversation

joshbuker
Copy link
Member

@joshbuker joshbuker commented Sep 29, 2016

Moved from #766 by @Tomomichi.

Still needs documentation and testing, help is welcome.

@joshbuker joshbuker added the help wanted label Sep 29, 2016
@Ch4s3
Copy link
Contributor

Ch4s3 commented Sep 29, 2016

If @Tomomichi doesn't have time I'll take a look this weekend.

@Tomomichi
Copy link

Tomomichi commented Sep 30, 2016

@Ch4s3 Sorry for taking this so long and thank you for your offer. That would be a great help.

@joshbuker
Copy link
Member Author

joshbuker commented Dec 5, 2016

@kyuden, @dankimio, this would be a good one to look at if you can.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Dec 29, 2016

@athix This looks pretty good. Do we want to roll this in under 0.10? Might make sense.

@joshbuker joshbuker requested review from Ch4s3, kyuden and dankimio Dec 29, 2016
Copy link
Contributor

@Ch4s3 Ch4s3 left a comment

if we can slap a test or 2 on this, then it's good to go.

@joshbuker
Copy link
Member Author

joshbuker commented Dec 29, 2016

@Ch4s3 wouldn't be a bad idea. Needs reviewed and conflicts resolved though.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Dec 29, 2016

@athix I fixed the conflict, and it looks fine, I'd feel better if it had a test, so we can give ourselves a bit of safety net.

@joshbuker
Copy link
Member Author

joshbuker commented Dec 29, 2016

@Ch4s3 Yeah, wouldn't want to push out a new submodule without decent testing in place first. Honestly, might be better to hold this off for a later release, just so it doesn't cause complications with the Rails 5 release in 0.10.0. Adding this submodule as part of 0.10.1 or 0.11.0 would work just as fine IMO.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Dec 29, 2016

I think that's a good call. Perhaps we can roll this in with any 0.10.0 bug fixes and call it 0.11.0. Then for 1.0 I want to start pulling the submodules out into independent gems.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Dec 29, 2016

@Tomomichi We haven't forgotten your PR. If you add some tests, we'll release it in the near future!

@Ch4s3
Copy link
Contributor

Ch4s3 commented Dec 30, 2016

Let's slate this for 0.11.0 and do some work on it soon.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Mar 24, 2017

@athix let's try to get this one moving again. I'll try to take a look asap.

@ebihara99999
Copy link
Contributor

ebihara99999 commented Sep 24, 2017

I started working on adding tests, but it seems to take long to complete.

@ebihara99999
Copy link
Contributor

ebihara99999 commented Oct 23, 2017

@athix
I've added specs of magic login submodules and would like to review them. But I don't know how to open a PR from others' working on and not completed.
If it's OK to open a PR as a new, I will do; If it's not, would you tell me the best way?
This is the branch working on.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Nov 28, 2017

@ebihara99999 are you asking if you can open a new PR with your work? If so, then yes of course you may! Please do in fact! Sorry for the delay.

Ch4s3
Ch4s3 approved these changes Nov 28, 2017
@ebihara99999
Copy link
Contributor

ebihara99999 commented Nov 29, 2017

@Ch4s3
Thanks! I opened a PR just now. Would you review it?
And thank you for dealing with many issues and PRs. I really appreciate it.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Nov 29, 2017

closing as #95 addresses this PR

@Ch4s3 Ch4s3 closed this Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants