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

C#8 simplified using to reduce nesting #8797

Merged
merged 1 commit into from Feb 25, 2021

Conversation

gerhardol
Copy link
Member

Follow up to #8729

Proposed changes

Some using statements missed in #8729 applied (using Intellisense suggestions only, no manual changes)
The second commit makes manual changes, to also use simplified using also if return is not in the using (so scope is marginally expanded).

Test methodology

Manual


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

@ghost ghost assigned gerhardol Jan 25, 2021
@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #8797 (2a4e15d) into master (b6905f1) will increase coverage by 0.03%.
The diff coverage is 31.75%.

@@            Coverage Diff             @@
##           master    #8797      +/-   ##
==========================================
+ Coverage   56.03%   56.06%   +0.03%     
==========================================
  Files         922      922              
  Lines       65944    65824     -120     
  Branches    12070    12070              
==========================================
- Hits        36951    36904      -47     
+ Misses      25985    25914      -71     
+ Partials     3008     3006       -2     
Flag Coverage Δ
production 43.29% <21.38%> (+0.03%) ⬆️
tests 94.83% <87.27%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@drewnoakes
Copy link
Member

I had started annotating the GitUI project. This is going to conflict pretty heavily. I hope to finish up that work this week.

Was this an automated refactoring? It should be, as far as I know. If so, can we hold off on merging this and do it again after I get the GitUI null annotations in?

@gerhardol
Copy link
Member Author

I can rebase this and #8789 on your PR (and wait with further #8788 PRs) until you are done.

#8789 is regex only (maybe a few manual touchups).
The first commit in this PR is invoking the Intellisense action manually /regex search), not so much time. I will be happy to find how to automate this...
The second commit is manual (return is outside the using{}, so the implementation may differ by a few assembly lines.
All is manageable.

@drewnoakes
Copy link
Member

Thanks @gerhardol. I'll try and finish up that PR this week. Turns out that annotating the GitUI assembly was quite challenging.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍

GitUI/GitUICommands.cs Outdated Show resolved Hide resolved
@drewnoakes
Copy link
Member

@gerhardol just to say that I am still working on this. I'm down to 59 annotation errors in 8 files within the GitUI project. The last ones are tricky, but I'm getting there. There's been some very loose and wild use of null throughout the code. It's satisfying to map this all out as a first step towards tightening some of these usages up.

@gerhardol
Copy link
Member Author

@gerhardol just to say that I am still working on this.

Thanks, I assumed that, annotation is a big change. I will see when you are ready!

@RussKie RussKie marked this pull request as draft February 17, 2021 12:42
@gerhardol gerhardol force-pushed the feature/c#8using branch 2 times, most recently from e0df4df to 93f2217 Compare February 24, 2021 09:06
@gerhardol
Copy link
Member Author

squashed comments and rebased after the GitUI annotations PR was merged. Some manual changes due to conflicts. Will squash the IntelliSense and manual commit at next update.
Drew has another annotations PR, I guess that should go first.

Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

Don't hold this up for #8878. It doesn't look like there'll be much in the way of conflicts. I will deal with any that arise 👍

@gerhardol gerhardol marked this pull request as ready for review February 24, 2021 11:25
Intellisense suggestions

Some manual changes

A few simplified object creations too
@gerhardol
Copy link
Member Author

squashed and rebased, no further changes

@gerhardol
Copy link
Member Author

@msftbot merge in 24 hours

@ghost ghost added the status: auto merge label Feb 24, 2021
@ghost
Copy link

ghost commented Feb 24, 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, 25 Feb 2021 20:13:16 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".

@RussKie RussKie merged commit f54a727 into gitextensions:master Feb 25, 2021
@ghost ghost added this to the 3.6 milestone Feb 25, 2021
@gerhardol gerhardol deleted the feature/c#8using branch February 25, 2021 07:13
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