Skip to content

Use pattern matching in PayloadHelper.cs to improve readability - #1003

Closed
sneznaj wants to merge 3 commits into
dotnet:mainfrom
sneznaj:main
Closed

sneznaj wants to merge 3 commits into
dotnet:mainfrom
sneznaj:main

Conversation

@sneznaj

@sneznaj sneznaj commented Dec 2, 2021

Copy link
Copy Markdown

Improves the readability using c# pattern matching
fixes #25

Improves the readability using c# pattern matching
fixes #25
@dnfadmin

dnfadmin commented Dec 2, 2021 •

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@sneznaj

sneznaj commented Dec 2, 2021

Copy link
Copy Markdown
Author

this was the last occurrence of this issue i found

return s_byteArrayTypeId;
}

if (type == typeof(int[]) ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this be included in the switch expression ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this is a super long logical expression that would not fit well into that format
Screenshot 2021-12-08 at 01 28 40

{
return s_timestampTypeId;
}
_ when typeof(IJvmObjectReferenceProvider).IsAssignableFrom(type) => s_jvmObjectTypeId,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think some of these lines are too long. Can you check https://github.com/dotnet/spark/blob/main/docs/coding-guidelines/csharp-coding-style.md ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

it is exactly at the limit
Screenshot 2021-12-08 at 01 23 42

@suhsteve

suhsteve commented Dec 6, 2021

Copy link
Copy Markdown
Member

Looks like builds are failing. Can you look into it ?

@sneznaj

sneznaj commented Dec 8, 2021

Copy link
Copy Markdown
Author

i fixed one reason and the other one was there before

@AFFogarty

Copy link
Copy Markdown
Contributor

Hi @sneznaj, are you still working on this?

@sneznaj sneznaj closed this by deleting the head repository Feb 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use C# pattern matching for matching types.

4 participants