embark-theme / iterm

An ambitious theme for iTerm
https://embark-theme.github.io/
26 stars 3 forks source link

Added the theme file and updated the readme with screenshots and info #1

Closed unknowledgeable closed 4 years ago

unknowledgeable commented 4 years ago

I'm just learning how git and github works so I hope this is what you expect!

skbolton commented 4 years ago

If you would like some coaching on git let me know. I am someone who leverages git in my workflow like crazy and I can say that it really is worth mastering git. Good job creating the PR and thanks for the changes. I will be able to squash your commits and clean things up, but some constructive feedback. I would suggest you look into rebasing and learn how to clean up your commits. Looks like there are 4 or so commits that are similar in concept (readme) stuff, but spread across several commits.

Mastering rebasing will completely change how you do git. Again if you would like coaching let me know.

skbolton commented 4 years ago

Also I will test this out tonight in iTerm to take a final look. Thanks for contributing!

unknowledgeable commented 4 years ago

Thanks for the feedback! I was thinking all those commits looked a bit silly but couldn't tell if that was normal or not.

I would suggest you look into rebasing and learn how to clean up your commits.

Thanks for the guidance, I'll read some tutorials on that before making any more PRs

Also I will test this out tonight in iTerm to take a final look. Thanks for contributing!

Some settings don't get stored in the .itermcolors file so make sure that they're set as follows so the colors work properly:

Might be worth mentioning that in the readme but I'll need to do my rebasing homework before pushing any more commits!

unknowledgeable commented 4 years ago

I'll take you up on a bit of coaching if that's ok!

So after a bit of reading here I did:

git rebase -i HEAD~6

I then put fixup before all the superfluous commits I made after "add theme and update readme"

and I get the following message

Successfully rebased and updated refs/heads/master.

git log --oneline now shows the following:

1219cf6 (HEAD -> master) add theme and update readme
65bb322 Initial commit

Which is what I think I want right?

Then git status gives me the following:

On branch master
Your branch and 'origin/master' have diverged,
and have 1 and 6 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

nothing to commit, working tree clean

Do I need to do anything special if I were to push this to github now? i.e. If I were to do git push origin master, would that clean up my fork and therefore the PR also?

If that is how it works, my understanding is that it would all have worked smoothly if I hadn't made the PR yet but it would likely fuck it all up if you'd already cloned and/or merged the fork by this point, right?

Thanks for the help!

skbolton commented 4 years ago

When you do a rebase you are essentially creating new commits. The list that pops up with the options to squash, fix up, edit, etc is your chance to change the commits. Either by melding them together, re wording messages, reordering commits, or even deleting whole commits. This is what makes rebasing powerful and it's also what makes it dangerous. When you do a rebase you are creating new history so you never want to do it with code you are sharing with others. Which is fine for feature branches and work that you are doing just like this pr.

However if you try to push now your upstream will complain that it has work that you don't have. This is because like I mentioned you created new commits when you rebase. The solution is to force push gut push -f. It's good that this sounds scary because you should never force push to shared code but in your case it's fine.

unknowledgeable commented 4 years ago

Ok great, I'll give it a go then! Fingers crossed haha!

skbolton commented 4 years ago

Congratulations @unknowledgeable on your first contribution! Thanks for the work.

unknowledgeable commented 4 years ago

Thanks!!

I'm not going to do it now because it looks like I might need to practice it a bit (and it's probably unnecessary) but would having one commit for adding the iterm theme and a separate one for for updating the readme be a more standard approach?

Would that be a good contender for a rebase split (assuming it were a more substantial contribution or the two patches were less related to each other)?

skbolton commented 4 years ago

Now that this has been merged it would be too late to do any changes. My take on how many commits to do is this. Usually when you merge a feature any work on the feature should be condensed down to a single commit. This article has a really good take on how to structure commit messages. If the feature has some refactor work that is not related to the feature then I usually move those commits to be before the feature work and then not squash everything down to a single commit. In my opinion its totally okay to merge several commits into master, but you have to have confidence in the person who makes the request that they don't have messy commits.