railsbridge / docs

Curriculum for RailsBridge workshops
docs.railsbridge.org
Other
3.41k stars 454 forks source link

Testing curriculum fixes #625

Closed akanshmurthy closed 6 years ago

akanshmurthy commented 6 years ago

@schaui6 @camillevilla

There were minor hiccups (items in console boxes that didn't need to be typed out, naming of tests, bolding hints that were missed, directory paths, test clarity) when we did this curriculum at the recent RailsBridge in SF. This PR addresses the minor hiccups (not sure why it's picking up so many changes... it's just the first 3 files after the Gemfile that I changed).

rachelmyers commented 6 years ago

@akanshmurthy, let's get a PR that only has the changes you want. You could clean this one up, or the easiest way might be to create a new PR. If you go that route, make sure you stash or reset any other changes you may have locally, pull the newest master branch, and then copy over the changes you want. Let me know if you run into trouble.

akanshmurthy commented 6 years ago

@rachelmyers : not sure what you mean. This PR does have the changes we want. If you're concerned with the number of commits in this branch, you can squash and commit before you merge?

rachelmyers commented 6 years ago

@akanshmurthy Did you mean for all the changes in the files changed tab to be included in this PR? The whitespace changes to the Gemfile jump out as something probably not intended. I meant let's get this to only include the changes you want to make, and not other changes.

akanshmurthy commented 6 years ago

@rachelmyers : sorry for the delay. I took a closer look at this and it seems like there's something wrong here. I believe that only the last commit (e40497f) is the one that needs to be merged as the rest of the commits are already in the master version of the code. I'll wait for @schaui6 @camillevilla to confirm before I move forward and open a new PR (and close this one).

It's also super hard to look at something that was written months ago and try to piece things together. It would be great if PRs could get reviewed in a timely fashion so that PR authors can remember what actually happened and new committers can be motivated to open a PR. When I've opened PRs in this repo, it's taken months (one time, I had to close a PR because it hadn't been reviewed in 7 months even after repeated bumps...).

camillevilla commented 6 years ago

@akanshmurthy thanks for the ping. I got caught up in the holiday rush this winter 😅 I'll have time to take a closer look this weekend and we can discuss what kind of rebase is in order.

camillevilla commented 6 years ago

hey @akanshmurthy, I did some rebasing and ironed out merge conflicts and pushed things up to akanshmurthy/docs#rebase-check. Please confirm that these are the changes you wanted to make: https://github.com/railsbridge/docs/compare/master...akanshmurthy:rebase-check

Once I get the thumbs up from you, I'll force push to akanshmurthy/docs#testing-curriculum-fixes and this PR will look nice n' neat for @rachelmyers review.

akanshmurthy commented 6 years ago

@camillevilla : no probs -> just to be clear, I was referring to a review from the maintainers of this repo not Sanderfer or you :)

In any case, yes. Your change is exactly what we need! Thanks so much!

camillevilla commented 6 years ago

Alrighty, force push is up @akanshmurthy! I'm always happy to provide a second set of eyes on testing curriculum stuff and make things easier for @rachelmyers to review.

akanshmurthy commented 6 years ago

Awesome, thanks a bunch! You rock!

akanshmurthy commented 6 years ago

@rachelmyers : thanks for the quick review! I was out on vacation this past week so I apologize for the delay on my end. But, I've addressed most if not all of your comments. Please take a look and let me know what you think; I should be pretty responsive as I'm back in the US now.

akanshmurthy commented 6 years ago

@rachelmyers : pinging to bump this :) we have a RailsBridge workshop at the end of the month and would like to have this merged in for that workshop.

rachelmyers commented 6 years ago

Ah, thanks for the bump; this looks good!

akanshmurthy commented 6 years ago

Thanks!