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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Got Emoji working #841

Merged
merged 0 commits into from Oct 6, 2013

Conversation

Projects
None yet
3 participants
@bjhaid
Copy link
Contributor

bjhaid commented Oct 6, 2013

I got emoji working 馃槃 leveraging on gemoji gem (it is only rails compatible), I am a newbie to open source so please be nice

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

Cool!

Would you mind rebasing and squashing? I'm not sure if you're new to git or only Open Source, so I'll walk you through it in case.

First, you need to have the upstream repository (kytrinyx/exercism.io) as a remote. I think you probably do because of the merge commits that I see.

git remote -v

This should show my repository as a remote. I'll assume it's named upstream in the following commands. If you've named it something else you'll need to replace upstream with your name.

Then, make sure you have the latest version of everything:

git fetch upstream

You need to be on the emoji branch:

git checkout emoji

Then, rebase from upstream's master:

git pull --rebase upstream master

If you look at your git log, you'll see that the merge commits that you had have disappeared.

Next I'd like the 4 commits to be a single commit, since they're really 1 thing.

This will kick off an interactive rebase back to the last commit before yours:

git rebase -i 01e179898cda0b31d46fe065eb488d53afb3e750

You'll get a screen that has your list of commits, like this:

pick b4f6365 added emoji
pick ee55c6b removed wrongly copied emojis
pick a9e295e got emoji working
pick 7157730 changed to_str to to_s

To squash the last ones onto the first ones replace pick with squash:

pick b4f6365 added emoji
squash ee55c6b removed wrongly copied emojis
squash a9e295e got emoji working
squash 7157730 changed to_str to to_s

When you write and quit the file, it will tell you that it is rebasing, and finally it will open the text editor with the following commit message:

# This is a combination of 4 commits.
# The first commit's message is:
added emoji

# This is the 2nd commit message:

removed wrongly copied emojis

# This is the 3rd commit message:

got emoji working

# This is the 4th commit message:

changed to_str to to_s

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.

It's actually much longer.

Now you can delete everything in the window and just type:

Add emoji

Write and quit the file, and you'll have a single commit with your changes.

To update the pull request, force push to your branch:

git push -f origin emoji

Let me know if you get stuck!

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 6, 2013

I have been using git for a while, but this is the first time I am doing real collaboration on git. Thanks for the knowledge shared.

I got it working but noticed the app doesn't start with:

gem 'rouge', github: 'rsslldnphy/rouge', branch: 'fix/table-inside-pre'

in the Gemfile

So I change that part to:

gem 'rouge'

to get the test environment running.

Thanks once again

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

We need this to be left as is, because it fixes a bug in how clojure code is displayed.

There are now very many commits that do different things. I'd like the pull request to be a single commit that represents only the emoji changes.

Can I help you get this sorted out?

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 6, 2013

Yeah thanks.

On Sun, Oct 6, 2013 at 2:09 PM, Katrina Owen notifications@github.comwrote:

We need this to be left as is, because it fixes a bug in how clojure code
is displayed.

There are now very many commits that do different things. I'd like the
pull request to be a single commit that represents only the emoji changes.

Can I help you get this sorted out?


Reply to this email directly or view it on GitHubhttps://github.com//pull/841#issuecomment-25774301
.

Abejide Ayodele
+234(0) 802 580 6662

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

OK, start by checking out the emoji branch and doing:

git pull --rebase upstream master

Then force push this up to your repository with:

git push -f origin master

Then we'll go from there.

@iHiD

This comment has been minimized.

Copy link
Contributor

iHiD commented Oct 6, 2013

@bjhaid - Great work on this. I'm wondering if these emoji are on a CDN or anything. 2,585 extra images is quite a big addition to the repo, and will dramatically slow down times to clone etc.

@iHiD

This comment has been minimized.

Copy link
Contributor

iHiD commented Oct 6, 2013

I'm also wondering if https://github.com/steveklabnik/emoji would be a good alternative, where the assets are bundled as part of that Gem, rather than exercism itself. Thoughts, @kytrinyx @bjhaid ?

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

I was looking into Sumo CDN which exists as an addon for Heroku. If steveklabnik/emoji have the gems bundled rather than including them I'd much rather go with that gem.

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 6, 2013

@kytrinyx done:

$ git pull --rebase upstream master
From github.com:kytrinyx/exercism.io
 * branch            master     -> FETCH_HEAD
Current branch emoji is up to date.
$ git push -f origin master
Everything up-to-date

@iHiD well I haven't found a cdn yet

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

That needs to be git push -f origin emoji not git push -f origin master

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 6, 2013

Done

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

Hm. Odd, I'm still seeing merge commits.

I've also been looking at the steveklabnik/emoji gem that @iHiD pointed out, and there's a lot that I like about it:

  1. It actually does the parsing, so we don't need to add helpers to exercism.
  2. It has tests (!!!)
  3. It has a Gemfile
  4. Steve is awesome :)
  5. We can put the images on a CDN without too much trouble.

Want to work with me on a version of this from scratch in a new branch? I'll just provide pointers and we can discuss here in the thread if you get stuck. I'll set up the CDN in the meanwhile.

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 6, 2013

Okay no problems

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

Cool (feel free to say no if you have better things to do, though, this issue can totally wait).

OK, so first step, make sure that your master is exactly like the upstream master:

git fetch upstream
git checkout master
git reset --hard upstream/master
git push -fu origin master

This will make your master branch both locally and on github match the upstream master.

Next, create a fresh branch:

git checkout -b skemoji # calling it sk for Steve Klabnik

That way the old branch is still around.

I'm going to create a tiny project that does only one thing: Serve emoji images. That way we can put that project on a CDN and then refer to that CDN from exercism.io.

@iHiD

This comment has been minimized.

Copy link
Contributor

iHiD commented Oct 6, 2013

馃挋 to both of you. Have fun!

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 6, 2013

@kytrinyx done

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

Cool. I'll keep working to figure out the CDN thing.

Would you try to get the emoji gem working with exercism.io?

Here's my suggestion:

  • Put the images in lib/app/public/img/emoji for now, but put that path in your .git/info/exclude file so it doesn't get committed by accident.
  • Create an environment variable called EMOJI_PATH, which you can set to /img/emoji for now
export EMOJI_PATH="/img/emoji"

Then use that environment variable in the application:

ENV.fetch('EMOJI_PATH')

Does that make sense?

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

It looks like we're going to want this version: gem 'emoji', '=1.0.0.pre'

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

I've asked the CDN folks some questions. I have to do some work, but will get back to this tomorrow.

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 6, 2013

Okay, would work on it later today and buzz you if I need a hand
On Oct 6, 2013 3:13 PM, "Katrina Owen" notifications@github.com wrote:

I've asked the CDN folks some questions. I have to do some work, but will
get back to this tomorrow.


Reply to this email directly or view it on GitHubhttps://github.com//pull/841#issuecomment-25775670
.

@kytrinyx kytrinyx merged commit 23bf4c8 into exercism:master Oct 6, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details
@iHiD

This comment has been minimized.

Copy link
Contributor

iHiD commented Oct 6, 2013

@kytrinyx - Did you just break Github?

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

Whut? I did not merge this!

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 6, 2013

23bf4c8 is Merge pull request #840 from etrepum/haskell-exercises, not emoji. I have no idea what happened.

@iHiD

This comment has been minimized.

Copy link
Contributor

iHiD commented Oct 7, 2013

/me is amused 馃憤

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 7, 2013

@kytrinyx I have had a look at emoji gem and found its assets(emojis) are not as much as the one I tried using earlier, the kind of parsing it has is not relevant in the current use case (unicode), it however helps in generating paths (if you consider this a valuable to require a gem for then I would else I wouldn't need a gem for this purpose), if I am missing things please let me know(as I would still need to parse the text to look for regex matching :emoji:)...

ATM revert back to the old code, making EMOJI_PATH(lib/app/public/img/emoji) an environment variable like you recommended, I would await your CDN tomorrow before I push my commit.

Thanks...

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 7, 2013

@bjhaid I haven't looked at this in depth, so I don't know what makes more sense.

I don't have strong feelings about which emojis we use, though I definitely want to use a CDN to serve the assets. (I haven't heard back from them, which is understandable since it is Sunday).

In short: as long as this is some sort of simple parsing that takes :heart: and turns it into an image tag that uses an environment variable which we can configure to point to a CDN to show 鉂わ笍, I think we're on the right track :)

Thanks for researching this!

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 7, 2013

@kytrinyx do keep me posted when you got the CDN all set up 馃挴 thanks

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 7, 2013

@bjhaid -- for sure, I'll keep you posted! For what it's worth, I'm still waiting to hear back from them.

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 12, 2013

no word yet :-(

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 13, 2013

No, I haven't heard back. If I haven't heard anything by Monday I'm just going to start making stuff up. Sorry :(

@kytrinyx

This comment has been minimized.

Copy link
Member

kytrinyx commented Oct 25, 2013

Update: I heard back from the CDNsumo folks. It's not straight-forward, and it will require some hacks. I'd rather not worry about emoji right now -- it's more trouble than it's worth for the moment.

@bjhaid

This comment has been minimized.

Copy link
Contributor

bjhaid commented Oct 25, 2013

Aww,

No problems, would look up issues I can help with. Thanks
On Oct 25, 2013 8:11 AM, "Katrina Owen" notifications@github.com wrote:

Update: I heard back from the CDNsumo folks. It's not straight-forward,
and it will require some hacks. I'd rather not worry about emoji right now
-- it's more trouble than it's worth for the moment.


Reply to this email directly or view it on GitHubhttps://github.com//pull/841#issuecomment-27090624
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment