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

Small bits #8996

Merged
merged 1 commit into from May 27, 2021
Merged

Small bits #8996

merged 1 commit into from May 27, 2021

Conversation

drewnoakes
Copy link
Member

Proposed changes

  • Fix annotations
  • Fix typos
  • Add words to dictionary to reduce warnings in the IDE
  • Remove redundant string interpolation

Test methodology

  • Manual review

✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned drewnoakes Mar 14, 2021
GitExtUtils/GitUI/Theming/AppColor.cs Outdated Show resolved Hide resolved
GitExtUtils/GitUI/IToolStripEx.cs Outdated Show resolved Hide resolved
@@ -73,7 +73,7 @@ public void Save(RepoDistSettings settings, IReadOnlyList<ExternalLinkDefinition
}
}

// TODO: refactor and outsource to the centralised SettingsSerialiser implementations.
// TODO: refactor and outsource to the centralised SettingsSerializer implementations.
Copy link
Member

Choose a reason for hiding this comment

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

AE spelling suggested...

Copy link
Member Author

Choose a reason for hiding this comment

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

I seem to remember checking that the new string matched an existing type name, but now I cannot find it. If this is supposed to be a type name, it should use cref.

Copy link
Member

Choose a reason for hiding this comment

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

Depends on when the comment was made :) If the type doesn't currently exist - then perhaps should say "..centralised settings serializer implementations."

GitExtensions.sln.DotSettings Show resolved Hide resolved
@@ -153,7 +153,7 @@ public new Font Font
public Action? OpenWithDifftool { get; private set; }

/// <summary>
/// Move the file viewer cursor position to the next TextMarker found in the document that matches the AppColor.HighlightAllOccurences/>.
/// Move the file viewer cursor position to the next TextMarker found in the document that matches the AppColor.HighlightAllOccurences.
Copy link
Member

Choose a reason for hiding this comment

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

I guess this meant something like

Suggested change
/// Move the file viewer cursor position to the next TextMarker found in the document that matches the AppColor.HighlightAllOccurences.
/// Move the file viewer cursor position to the next TextMarker found in the document that matches the <see cref="AppColor.HighlightAllOccurences"/>.

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 18, 2021
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 18, 2021
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

A few of RussKies comments remain, that make sense
OK for me

@RussKie
Copy link
Member

RussKie commented Apr 24, 2021

Please rebase and squash merge

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Apr 24, 2021
@ghost ghost added the 💤 status: no recent activity The issue is stale label May 24, 2021
@ghost
Copy link

ghost commented May 24, 2021

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs.

@gerhardol
Copy link
Member

@drewnoakes Do you want someone else to rebase and squash this?

@ghost ghost removed the 💤 status: no recent activity The issue is stale label May 24, 2021
@drewnoakes
Copy link
Member Author

@drewnoakes Do you want someone else to rebase and squash this?

@gerhardol thanks for following up. I'm swamped with work right now so won't get to look at this any time soon. If someone wants to pick it up and merge it, they're most welcome to. I would also understand if this is closed.

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label May 25, 2021
@gerhardol
Copy link
Member

Rebased with conflict resolve - plan to squash to one commit and merge tomorrow

* Annotations

* Remove redundant string interpolation

* Fix various spelling mistakes

* Add words to the project's dictionary
@gerhardol
Copy link
Member

@msftbot merge in 24 hours

@ghost ghost added the status: auto merge label May 26, 2021
@ghost
Copy link

ghost commented May 26, 2021

Hello @gerhardol!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Thu, 27 May 2021 20:25:26 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@gerhardol gerhardol merged commit d53068b into gitextensions:master May 27, 2021
@ghost ghost added this to the 3.6 milestone May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants