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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typos #237

Merged
merged 1 commit into from
May 25, 2022
Merged

Typos #237

merged 1 commit into from
May 25, 2022

Conversation

danielbanda
Copy link
Contributor

Closes #85

Fixed some documentation typos 馃槈

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • Tested code with current supported SDKs
  • New component
    • Pull Request has been submitted to the documentation repository instructions. Link:
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes
  • Every new API (including internal ones) has full XML docs
  • Code follows all style conventions

@net-foundation-cla
Copy link

net-foundation-cla bot commented May 4, 2022

CLA assistant check
All CLA requirements met.

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Looks great! Couple of small nits:

  • Is the linked issue wrong? That seems unrelated to the changes here 馃
  • Could you rebase and drop the top two commits (the accidental one and the one just reverting it?). I'd rather just have a single clean commit in the PR.

Thank you! 馃槃

@Nirmal4G
Copy link
Contributor

@Sergio0694 You could squash merge to get a single clean commit too!?

@Sergio0694 Sergio0694 added maintenance 鈿欙笍 Some regular maintenance updates mvvm-toolkit 馃О Issues/PRs for the MVVM Toolkit labels May 21, 2022
@Sergio0694
Copy link
Member

@Nirmal4G We're trying to just merge PRs normally, as that preserves the history better. Squashing still preserves the individual commits, which in this case are just not meaningful. I'd rather just have PRs only keep the commits they need. This is also why eg. when doing lots of trial and errors, I'd rather just squash those commits and force push to the PR branch, and then merge that PR normally (so that all commits are kept but you still don't have a dozen test commits that are just clutter).

I'll keep this open for a few days in case Daniel gets back, if not I'll close and cherry-pick this commit manually 馃檪

@Nirmal4G
Copy link
Contributor

@Sergio0694 What you described is rebase merge. Squashing takes all the changes from the tip and combines into a single commit on top.

...preserves history better...

All merge strategies provided by github PR flow preserve history except rebase merge (This behavior can be changed) but only the way they merge is different. You can configure merge options in repo settings.

Squash and Merge your PR commits

Personally, I prefer linear history on master branch but that's just me! 馃槆 But for some PRs under community toolkit, especially mine (I always specify what merge strategy should be used) and some others I've noticed, sometimes squashing or rebasing works best. Merge commit doesn't always preserve history properly.

For instance, Blame gets messed up if you include lines that have cascading changes, diff into completely different than viewing individual commits. For this reason, I always recommend against merge commit. Use merge commit only when you want a branch with an explicit patch tree (that doesn't contain cascading changes on the same lines) else do a squash merge or rebase merge.

@danielbanda danielbanda mentioned this pull request May 25, 2022
12 tasks
@Sergio0694
Copy link
Member

@danielbanda leaving instructions here for what to do since I can see it might not have been clear.
If what I asked wasn't clear though, just feel free to ask next time 馃檪

"Could you rebase and drop the top two commits (the accidental one and the one just reverting it?). I'd rather just have a single clean commit in the PR."

What you need to do is the following:

  • git rebase -i HEAD~3
  • Change the top 2 commits (the accidental one and the revert) to drop
  • Save and exit. If vim opened for the rebase and you're not familiar with it, do this:
    • Press "i" to enter "insert" mode. Move to the top two commit and change them to "drop".
    • Press ESC, then type ":wq" and ENTER to save and exit.
  • git push --force origin <YOUR_BRANCH_NAME> to force push

After this, the PR will just have the first commit nice and clean, with nothing else 馃憤

@danielbanda
Copy link
Contributor Author

HI @Sergio0694, thanks to your thorough instructions I was able to rebase the PR to a single commit.

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

Thank you for taking the time to properly rebase this! 馃槃

@Sergio0694 Sergio0694 merged commit d4a8971 into CommunityToolkit:main May 25, 2022
@Nirmal4G
Copy link
Contributor

@danielbanda The fixes issue mentioned might be wrong, since it closed my PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance 鈿欙笍 Some regular maintenance updates mvvm-toolkit 馃О Issues/PRs for the MVVM Toolkit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants