Skip to main content

5 Open Source Vlog - Housekeeping and Merging Pull Requests

Episode 63 · June 25, 2015

Cleaning up our open source pull requests for simple_calendar

Open Source


Transcripts

What's up guys? This is day two of the open source vlog that I'm doing, yesterday we talked about where do we start. We looked at rails, we looked at devise, we didn't really find anything that we could contribute to, at a glance at least, I think we can take a look at rails and devise maybe every day or something like that, and just see what's out there. So maybe we stumble into something that's a new ticket that we can maybe stumble on and maybe we can solve it, we just have to kind of keep an eye on those tickets. So let's just do a quick glance through here and see if we can find anything here. I feel like we probably won't, or that it would take a lot of effort into solving.

I feel like trying to dive into rails just at the beginning here is going to take some time. So let's take a look at devise and then get back to simple calendar, which we decided to tackle yesterday. So it doesn't seem like there's anything new on devise's open issues, so we can get back to simple_calendar, now I will say that last night. This morning I got an issue here from Rodrigo, and he was working on getting our spec working, and he submitted a pull request a few hours ago. He added this /bin/setup and /bin/console so basically, what he's trying to do is set this up so that you can test simple_calendar easier. I've actually written zero tests for simple_calendar and I think that would be awesome to get done on the gem, so maybe we'll dive into that with him, because it seems like a really great starting point. If you find a gem with no tests, there you go, that's an easy thing to start working on. Adding tests, even the most basic tests are that are like: When I require the gem I should be able to access this class. Those tests are valid so those are useful to add. Let's take a look at the code that he wrote. If you see the conversation, he basically just say: "Adding these will simplify a lot for people who want to debug or test simple_calendar, recommend adding custom console attached in the request". So this /bin/console file that he added works really similar to rails's console. It loads up bundler. It loads up rails, which is the dependency for simple_calendar, and then requires irb and starts an irb session. And since it's ruby, it's already loaded the simple_calendar gem in rails, and that's it, you should be good to go, and bin/setup will install the dependencies. So that would basically install rails if you didn't already have it, seems good. It's really straightforward code, I don't see anything wrong with it, now we could just go an merge this, by clicking the "Merge Pull Request" button, but we actually want to test this out on our machine first to make sure that it works. GitHub adds some awesome instructions here if you click on the command line. They show you: If you've already checked out your repository, you can check that out, that's all you have to do, it's really simple to follow these two steps. Let's jump into the terminal and do this.

What happens here is: 1) It checks out a new branch with the user's name-their branch's name: So "rodrigo-master", we're going to make a branch on our local machine that's called that, and it's going to mirror our master branch, because that's where we would like to merge it, and then the next command is going to pull that branch from his Git repository to our new branch, and then we'll be able to test it out, then if it works, we check out our master branch merge his master branch into it, and then we can push it up to GitHub, and then it's officially done. Let's dive into that.

We've got simple_calendar checked out on my machine, we've just got to paste this in. That was it, as simple as that, we are checked out into the Rodrigo master branch, and if we list the files here, you'll see a bin folder here, you can go into bin, we can run ./bin/console, we have kind of a weird homebrew warning message. We've got a weird homebrew set up on this machine that's shared across multiple user's because it's a shared office mac, so don't worry about that. Here we should be able to access the simple_calendar module. And we can, so this works perfectly. We can try the ./bin/setup, this will just run bundler as you would expect. It loads up rails which has a lot of dependencies as it is and simple_calendar, and we're done. I think it's good to merge. We could just click "Merge Pull Request", but it's almost always better to do these things on your terminal. Get full control over, so you can do it the right way, and this is something that regardless of using GitHub, maybe you're on BitBucket or your own Git server, anything that you're using: Follow these instructions generally because they're good practice, and even if you don't have this "Merge Pull Request" button, it's good practice to learn how to use Git appropriately. What happens here is if we git checkout master, we jump from Rodrigo's branch to our master branch, then when we merge, we're going to do git merge --no-ff so this is basically going to create a commit here. I had a bit of a problem, Vim didn't provide the right exit status or something, when I did this Git merge, and so when I quit Vim here and save this, it thought that Vim has been closed improperly, so I just went and googled that and cut that out from this episode because it wasn't really relevant, but if you ever run into this, just Google it, "There was a problem wit the editor Vi", that was it, I just ran the git config --global core.editor /usr/bin/vim and we're back in business. It's kind of irrelevant in the weeds, but if you run into those things, and I'll try not to cut that out next time, but if you run into those things, usually you can just google it and find out more information on it and probably it does those solutions pretty quickly.

What happened this time, now that I did the git merge --no-ff it successfully went through. If we look at our git logs, we'll see that we now have my last commit, which is down here, and we merged the pull request. And Rodrigo had two commits for his, and we have one merge commit, so he owns these commits and I merged them in to that master branch and really the last thing we have to do is git push origin master and if we do -u I think this sets this the default like remote branch if you were just to run git push with nothing else.

With that said, we're done, and actually, without even refreshing our page on GitHub, they've merged and marked the pull request as merged, which I always love because it's real time updating now, and they already know the status of these things before you can even tell them, it's awesome. GitHub is fantastic.

We've merged the pull request and Rodrigo shows up in the Git history, which is super cool, along with everybody else that's contributed, and we don't really need to release a new version of this, this is still like in progress. There's no benefit to anybody if we were to release a new version of this on ruby gems, so the latest version that I have here is 1.1.10. Since this doesn't affect functionality of the app at all, this is more for development stuff, we don't really need to release a new version of that. If we do add a feature in the future that affects the way that things work, we can decide how long do we want to wait until we release a new version or not. I have a tendency to just release lot's of new versions, as long as they don't break anything. I think I'm going to call it a day, we've got a pull request merged, maybe we'll take a look at these and see if any of these old ones are worth closing. Some of them are probably old, so this adds a start date to the default classes on the the TD tags.

We haven't really talked about this, the simple_calendar generates a table, and HTML table. It has a header, it has multiple rows for however many weeks in the calendar, and then each of the days is a td tag. So I have this semi long line of code here that will make an array of classes for the td. This is helpful for when you're writing CSS and you want to say: This day is in the past, this day is in the past, this day is in the last month and it's in the past. This day is in the future, this is the current day, this is whatever. So we have lots of these, we have the previous month, current month, next month, and the start date is kind of useful for people I'm sure, and it probably doesn't really hurt to add this. It basically is like the start date is this special day. You can pass in the date that start date is what determines which month to display, so that's like the current day. We may want to do that, also, I'm not sure that that's the same as today, so we have the current calendar we should look at this and see if it's the same idea.

Today is Time.zone.no.to_date, and the start date is actually the date from the params. In the url you pass in a day, and that day gets pulled in and it's what detemines how the calendar displays. If you click next month or next view, then that start date will change and it will be the first day of that next like month, or if it's a two week calendar it'll be the first day of the next two weeks, so the start date actually makes a lot of sense, it won't affect them but it doesn't hurt to have this in there, so I kind of like it. Let's merge it, and I feel terrible because this was a long time ago, and I never merged it, I'm going to be lazy this time, and we're just going to "Merge Pull Request" because there wasn't anything to test, so this seems fine. Let's just do that, call that a day, and get back to working on stuff. I think that's totally fine, there's more to this one, so maybe we'll review this later both of these, or all three of these actually have a little bit of conversation on them, so let's actually dive into this tomorrow. Let's take a look at those.

(14:55) Clearly I'm not doing a great job of keeping track of these pull requests, but I usually try to make sure I comment at least once. So I answer this guy's question, don't see any response to it so I think I'm going to close this issue and clean this out, so I'll assume that we did what we wanted and we'll get back to work, so one day per page, same thing, I answered the guy's question, do a little housekeeping today before we call it a day.

This one I had a long conversation... Looks like we got everything working on that one, so let's close that one too. I clearly need to do a better job of keeping track of these, so let me refresh and update this.

"month_calendar generates date strings in <td>'s instead of expected display". This actually looks like it rendered out properly as a table, there's just no styling on it, so I believe that's totally fine. But regardless, so I can assume that it's fixed.

What else do we got? Making your own view helper class, this one is called mini_calendar and then you can pass in those td classes as an override. I definitely need to spend some time redoing the API here. The API here is terrible, you have to pass in a proc or a lambda or a block of some sort that can be called later. This is pretty gnarly because when you've got to generate a method because you don't want to put this in line, that would be terrible, that's too much code, and you don't want to put it in line and then you have all this like this. This is a terrible way to interface with the calendar and I really really need to clean that up... If we look at issue 67... actually that's the same one. The ticket references itself. So we've added the pull request to do start time before. We can just close this because the pull request happened. I'm just going to say: "Sorry for being so late on this, I merged your pull request and should release that in a version shortly". I feel bad to being so super late to that, but it is what it is.

"Parsing attribute: :start_time causes error". We talked about this a long time, apparently I asked for example code and didn't get anything, so we can assume that that is no longer relevant as well.

So this last ticket that's open was my fault, I need to allow passing falses in the option as a boolean instead of a string that's false. Right now you can pass in an empty string if you want to remove the previous month link, or the next month link, and that's fine... But ideally it's better if you pass in false it just doesn't display it. That makes more sense to people, and it's a lot more logical, but for now I think the empty string is probably acceptable. So let's just close that too, and today ended up being taking a look at the issues and doing some housekeeping, and tomorrow let's start with our spec. Let's start by trying to write some tests for this and get that set up. We've gotten our specs file. It doesn't really do anything, we had this commit back in the day, we had actually a spec folder at some point, where did our spec folder go? I did remember that commit and it's gone now. Now that we've gotten this housekeeping done, it's time to actually figure out the plan, what we're going to work on, start working on it. Outline some of the to-dos and see if anybody else wants to jump in and hack on this with me, so let's do that tomorrow. Peace v

Transcript written by Miguel

Discussion