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

Update CSharpier to 0.30.3 #2912

Merged
merged 5 commits into from
Dec 17, 2024
Merged

Update CSharpier to 0.30.3 #2912

merged 5 commits into from
Dec 17, 2024

Conversation

pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Dec 17, 2024

This PR:

  • Updates CSharpier to 0.30.3, which performs these changes:
    • Enforce trailing commas for multi-line object and collection initializer
    • Remove training commas for for singleline object and collection initializer
    • Fix blank line being added with lambda returning collection expression
    • Fix inconsistent Formatting for new() Operator Compared to Explicit Object Constructor
  • Updates .editorconfig to match CSharpier's suggested configuration
  • Enable collection expressions by default via dotnet format

If have broken these changes into separate commits in case you think collection expressions are a bad idea and I need to revert it (I personally prefer the syntax and use it when updating code or writing new code).

I figure now is the best time for a change like this, as a major update to the C# code base (SF-2900) has been merged.


This change is Reviewable

src/SIL.XForge/Realtime/RichText/Delta.cs Fixed Show resolved Hide resolved
src/SIL.XForge/Realtime/RichText/Delta.cs Fixed Show resolved Hide resolved
src/SIL.XForge/Realtime/RichText/Delta.cs Fixed Show resolved Hide resolved
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 70.78189% with 71 lines in your changes missing coverage. Please review.

Project coverage is 80.84%. Comparing base (86eaa35) to head (7ab3999).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...c/SIL.XForge.Scripture/Services/ParatextService.cs 76.19% 7 Missing and 3 partials ⚠️
...e.Scripture/Controllers/SFProjectsRpcController.cs 25.00% 9 Missing ⚠️
...ture/Services/JwtInternetSharedRepositorySource.cs 0.00% 7 Missing ⚠️
src/SIL.XForge/Realtime/Json0/Json0OpBuilder.cs 36.36% 7 Missing ⚠️
...XForge/Services/AuthServiceCollectionExtensions.cs 0.00% 6 Missing ⚠️
.../SIL.XForge.Scripture/Services/SFProjectService.cs 61.53% 5 Missing ⚠️
...rge.Scripture/Services/SFInstallableDblResource.cs 57.14% 1 Missing and 2 partials ⚠️
src/SIL.XForge/Controllers/UsersRpcController.cs 25.00% 3 Missing ⚠️
...rc/SIL.XForge.Scripture/Services/DeltaUsxMapper.cs 85.71% 1 Missing and 1 partial ⚠️
src/SIL.XForge/DataAccess/MemoryRepository.cs 50.00% 1 Missing and 1 partial ⚠️
... and 15 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2912      +/-   ##
==========================================
+ Coverage   80.81%   80.84%   +0.02%     
==========================================
  Files         532      532              
  Lines       31128    31107      -21     
  Branches     5054     5062       +8     
==========================================
- Hits        25156    25148       -8     
+ Misses       5221     5208      -13     
  Partials      751      751              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

:lgtm: with two comments. I'm fine with merging; just wanted to leave some opinions.

Reviewed 93 of 93 files at r1.
Reviewable status: 92 of 93 files reviewed, 5 unresolved discussions (waiting on @pmachapman)


src/SIL.XForge/DataAccess/MemoryRepository.cs line 171 at r1 (raw file):

    public Task<int> DeleteAllAsync(Expression<Func<T, bool>> filter)
    {
        T[] entities = Query().Where(filter).ToArray();

Personally I think a method like ToArray() makes a lot more sense than using a spread operator because it's easier to follow a chain of methods, than syntax that alternates between chaining and whatever the opposite of chaining is (I consider a().b().c() to b easier to follow than c(b(a()))).


src/SIL.XForge.Scripture/Services/ParatextService.cs line 285 at r1 (raw file):

                using ScrText scrText =
                    ScrTextCollection.FindById(username, paratextId)
                    ?? throw new Exception(

I'm not sure the x = y() ?? throw new Z() pattern is a good idea...

Copy link
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

Reviewable status: 92 of 93 files reviewed, 5 unresolved discussions (waiting on @Github-advanced-security[bot] and @Nateowami)


src/SIL.XForge/DataAccess/MemoryRepository.cs line 171 at r1 (raw file):

Previously, Nateowami wrote…

Personally I think a method like ToArray() makes a lot more sense than using a spread operator because it's easier to follow a chain of methods, than syntax that alternates between chaining and whatever the opposite of chaining is (I consider a().b().c() to b easier to follow than c(b(a()))).

Yeah I was initially hostile to it, but since TypeScript uses the similar [...otherArray], it has grown on me.


src/SIL.XForge.Scripture/Services/ParatextService.cs line 285 at r1 (raw file):

Previously, Nateowami wrote…

I'm not sure the x = y() ?? throw new Z() pattern is a good idea...

Yeah that was unexpected from dotnet format.

I believe it is more efficient in the generated MSIL code, as is it ensures that the finally { scrText.Dispose(); } which the using does in the background is not executed.

src/SIL.XForge/Realtime/RichText/Delta.cs Fixed Show resolved Hide resolved
src/SIL.XForge/Realtime/RichText/Delta.cs Fixed Show resolved Hide resolved
src/SIL.XForge/Realtime/RichText/Delta.cs Fixed Show resolved Hide resolved
Copy link
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

Dismissed @Github-advanced-security[bot] from 3 discussions.
Reviewable status: 91 of 93 files reviewed, 2 unresolved discussions (waiting on @Nateowami)

@pmachapman pmachapman merged commit f07ba36 into master Dec 17, 2024
16 of 17 checks passed
@pmachapman pmachapman deleted the update/csharpier branch December 17, 2024 02:20
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.

2 participants