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

Updates welcome message - #82

Open
ksmacleod99 wants to merge 15 commits into
OperationCode:masterfrom
ksmacleod99:master
Open

Updates welcome message#82
ksmacleod99 wants to merge 15 commits into
OperationCode:masterfrom
ksmacleod99:master

Conversation

@ksmacleod99

@ksmacleod99ksmacleod99 commented Oct 1, 2017

Copy link
Copy Markdown

Description of changes

Proposed updates to Slackbot welcome message

Issue Resolved

No existing issue

hollomancer
hollomancer suggested changes Oct 7, 2017
View reviewed changes

@hollomancerhollomancer left a comment

Copy link
Copy Markdown
Member

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

hpjaj commented Oct 8, 2017

Copy link
Copy Markdown
Collaborator

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

Hiren-Mehtaand 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
Conrad Hollomon@ksmacleod99
test what's going on here
db51c36
Conrad Hollomon@ksmacleod99
this test really needs to change
9d0212b
Conrad Hollomon@ksmacleod99
Fixed tests
aa7004a
Conrad Hollomon@ksmacleod99
Update Dockerfile
cce802a
Conrad Hollomon@ksmacleod99
Update to Ruby 2.3.5
a85ed5e
Conrad Hollomon@ksmacleod99
stop docker from caching on travis?
335b1c5
Conrad Hollomon@ksmacleod99
Oh snap I bet it's this
fe60179
Conrad Hollomon@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

jjhampton commented Oct 10, 2017

Copy link
Copy Markdown
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

ksmacleod99 commented Oct 10, 2017

Copy link
Copy Markdown
Author

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

@jjhampton

jjhampton commented Oct 10, 2017•
edited
Loading

Copy link
Copy Markdown
Member

@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

ksmacleod99 commented Oct 11, 2017

Copy link
Copy Markdown
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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@hollomancerhollomancerhollomancer requested changes
Reviewers whose approvals may not affect merge requirements

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