Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multiple problems: Remove non-ASCII characters to fix #441 #527

Closed
wants to merge 1 commit into from

Conversation

robkeim
Copy link
Contributor

@robkeim robkeim commented Jan 30, 2017

@Insti Insti changed the title Remove non-ASCII characters to fix #441 multiple problems: Remove non-ASCII characters to fix #441 Jan 30, 2017
@Insti
Copy link
Contributor

Insti commented Jan 30, 2017

I'm not sure putting changes to multiple problems in the same commit is a good idea.

Are you sure that these tests are not also testing another aspect of the solution?

Edit: These changes look fine, so maybe we should just try it and see.

@robkeim
Copy link
Contributor Author

robkeim commented Jan 30, 2017

@Insti should I create three different PRs then? (edit: I meant commits not PRs)

I looked through the tests and given the titles they are explicitly testing non-ASCII functionality. I don't have any way of validating if the original intent of the test was to cover two different things though.

@Insti
Copy link
Contributor

Insti commented Jan 30, 2017

3 commits would be good.

It's good to ask yourself, "Might I need to revert part of this?".

@robkeim
Copy link
Contributor Author

robkeim commented Jan 30, 2017

Ok sounds good @Insti

I've got a couple of follow-up questions:

  • What is the best way to update this to 3 separate commits? Do I simply abandon this PR and start a new one, or is there some ninja git trick I'm missing?
  • Don't we do a "squash and merge" when merging into master? In that case, the intermediate commits are going to be lost right?

@petertseng
Copy link
Member

What is the best way to update this to 3 separate commits?

The way I like to do this is git reset origin/master, then use git add -p to add each change separately, with git commit for each commit. You will need to push --force when done.

Don't we do a "squash and merge" when merging into master?

I would recommend not doing so if there are separate commits that are meant to stay separate. Only squash and merge if there is one commit in the PR, or if the commits in the PR are meant to be squashed.

@robkeim
Copy link
Contributor Author

robkeim commented Jan 30, 2017

Thanks @petertseng for the tip, although the forced push didn't go well so I had to delete the remote branch and start again :(

The new PR is #529

emcoding pushed a commit that referenced this pull request Nov 19, 2018
…properly

Simplecov needs to be included *before* minitest is required.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants