Skip to content
This repository was archived by the owner on Sep 4, 2018. It is now read-only.

Updates welcome message #82

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Updates welcome message #82

wants to merge 15 commits into from

Conversation

ksmacleod99
Copy link

Description of changes

Proposed updates to Slackbot welcome message

Issue Resolved

No existing issue

hollomancer
hollomancer suggested changes Oct 7, 2017
View reviewed changes
Copy link
Member

@hollomancer hollomancer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya, can you remove .DS_Store and views/.DS_Store from the PR?

@hpjaj
Copy link
Collaborator

hpjaj commented Oct 8, 2017

@ksmacleod99 - Once PR #86 is merged, and you rebase on master, you should no longer have failing tests.

Hiren-Mehta and others added 14 commits October 10, 2017 12:07
@Hiren-Mehta @ksmacleod99
Update mentorship_message.txt.erb
24a0813 
Changed a few words and the link based on the currently in-use form.
@ksmacleod99
Delete .DS_Store
3e4207a
@ksmacleod99
Delete .DS_Store
f280b70
@ksmacleod99
test what's going on here
db51c36
@ksmacleod99
this test really needs to change
9d0212b
@ksmacleod99
Fixed tests
aa7004a
@ksmacleod99
Update Dockerfile
cce802a
@ksmacleod99
Update to Ruby 2.3.5
a85ed5e
@ksmacleod99
stop docker from caching on travis?
335b1c5
@ksmacleod99
Oh snap I bet it's this
fe60179
@ksmacleod99
Okay I was in the ballpark
7c65390
@hpjaj @ksmacleod99
Updates ruby version to be in keeping with gemfile
f60b81b
@hpjaj @ksmacleod99
Updates .gitignore to ignore .DS_Store
6f15f16
@hpjaj @ksmacleod99
Removes expectation of a precise message
c87a6b3 
The tests still expect the `new` method to be called, and to return a value.

It simply no longer strictly expects the `new` method to be called with a specific paragraph of text.

This allows for flexibility in changing the text of the message, while still testing that the methods are being called.
@jjhampton
Copy link
Member

It seems like some commits in this PR (f280b70 thru c87a6b3) are duplicates of commits that are already in master. Seems like this somehow happened during rebase?

@ksmacleod99
Copy link
Author

I have no idea. How can I clean this up and get it to work properly?

@jjhampton
Copy link
Member

jjhampton commented Oct 10, 2017 •
edited
Loading

@ksmacleod99
Ideally, a PR should be as lean as possible, and only contain the changes you're proposing to merge in. It's not the biggest deal in the world, but the duplicate commits contain the same changes that other people have already added (just with new commit hashes), which could be confusing for someone else trying to read thru the commit history in a linear fashion and figure out what happened, when, and why.

So, your branch (ksmacleod99:master) really only needs to contain the commits 0152753 (Updates welcome message) and 3e4207a (Delete .DS_Store) that you actually added. Somehow during your rebase/merge/force-push, those other commits (f280b70 thru c87a6b3) got caught in there.

No worries tho! Luckily, you can easily use Git's interactive rebase feature to clean up your commit history that you are proposing to merge in w/ this PR. I would be happy to walk you through the process.

@ksmacleod99
Copy link
Author

@jjhampton Yes, that would be great! I have no idea how I did that (but I also had no idea how to rebase, so that might be the problem). I need to call it an evening; but when would be a good time for you?

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

@hollomancer hollomancer hollomancer requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ksmacleod99 @hpjaj @jjhampton @hollomancer @Hiren-Mehta