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

Add missing tests based on coverage report #164

Closed
wants to merge 0 commits into from

Conversation

silkentrance
Copy link
Collaborator

@silkentrance silkentrance commented Dec 7, 2017

The coverage report from #107 shows that we are missing some tests.

This PR is about implementing additional tests so that we can increase our overall test coverage.

TODO

  • invalid template option
  • valid template option with pre existing file or dir
  • TBD

@silkentrance
Copy link
Collaborator Author

@raszi never ever merge into existing PRs. this breaks the whole work flow and all the other PRs. You just increased the amount of work by a manifold. Yikes.

@silkentrance
Copy link
Collaborator Author

silkentrance commented Dec 26, 2017

@raszi rebase is our friend here. rebase+merge gets the job done. and merge and then rebase will break everything, unless it is not done on the originating branch. AND NEVER THE GITHUB INTERNAL PR BRANCH, DAMNIT.

@silkentrance
Copy link
Collaborator Author

silkentrance commented Dec 26, 2017

@raszi I do not know what you did to master but all my rebases here are failing with arbitrary merge conflicts and out of line/weird merge proposals. Did you push --force to master?

@silkentrance
Copy link
Collaborator Author

@raszi please use rebase wherever possible. I am a having a hard time with gh-121 and gh-155. while both have been resolved for now, merging these into master will take extra work...

@silkentrance
Copy link
Collaborator Author

wtf. is github bonkers or what? this cannot be merged, never.

@silkentrance
Copy link
Collaborator Author

@raszi for the record: I did not close this and i would have never merged into master.
This leaves the question: who did close and merge this?
image

@silkentrance
Copy link
Collaborator Author

@raszi seems as if this was closed (again) due to my latest push --force attempt to fix existing issues in the branch. Yikes. Github is becoming somewhat intransparent right now.

@raszi
Copy link
Owner

raszi commented Dec 27, 2017

@silkentrance I strongly disagree with your view and so does Linus Torvalds.

We should not fake the history and since this is not a local branch of yours and somebody else could use it therefore you should not rebase the changes.

Merging the recent changes is a task what you did and therefore it should appear in the history. You should rather merge the changes, resolve the conflicts and push the merge commit to the remote branch. GitHub will only show the differences and of course since it won't have conflicts the branch will be mergeable.

@raszi
Copy link
Owner

raszi commented Dec 27, 2017

Long story short, you should only rebase your local changes and never rebase anything what you've already pushed to a remote branch.

@raszi
Copy link
Owner

raszi commented Dec 27, 2017

This PR is borked for some reason. GitHub shows no commits and no changes. 😟

@raszi
Copy link
Owner

raszi commented Dec 27, 2017

You did a couple of force pushes, which could mess up GitHub. I tried to recover the PR from my local git repository, see #165

@raszi raszi deleted the add-missing-tests branch March 13, 2018 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants