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

Enhance Code Readability #84

Closed
wants to merge 28 commits into from
Closed

Enhance Code Readability #84

wants to merge 28 commits into from

Conversation

nimanikoo
Copy link

Hello, I hope you are well
Thank you for helping to open source and build this project for the .NET community
Out of curiosity, I browsed through the code in this repository and found some hints that might be good.
One of the performance improvements was where you used lists.
I tried to use the new features of .NET 8 and C# 12 and refactored where needed
In the commits, I stated the reason for the changes and on what basis the changes were made.
In some files, it was necessary to sync namespace , and I think namespace scope can be used for all files, according to the rest of the new .NET projects such as Aspire.
The next thing is that you can use the primary constructor according to the new .NET 8 feature
In some cases, we can make the code more concise and readable and use expressions.
If this model of changes is approved by you, let me know so that I can apply it to the whole project and move forward step by step.
I hope these things are useful and I will be happy to contribute to the development of this project
Please check and let me know the result

Copy link
Contributor

@PaulusParssinen PaulusParssinen left a comment

Choose a reason for hiding this comment

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

Not a maintainer but otherwise I agree with all other changes 👍

@DUWENINK
Copy link

I vote for this

Copy link
Contributor

@PaulusParssinen PaulusParssinen left a comment

Choose a reason for hiding this comment

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

Didn't suggest all rename changes so please go through the constants in LightEpoch again to make them consistent with my previous suggestion, otherwise LGTM :shipit:

Copy link
Contributor

@JeremyWu917 JeremyWu917 left a comment

Choose a reason for hiding this comment

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

I don't know if it makes sense, but it looks coooool 😎

@douglasg14b
Copy link

What is the appetite for automatic styling? Many of these things can be linted for and even auto-fixed to some arbitrary level of consistency without requiring human intervention.

@nimanikoo
Copy link
Author

I appreciate your point, but I don't think we can ignore the impact of human creativity on coding.
You're right that automation can make many tasks easier and faster.
However, When it comes to code refactoring and using new .NET features, I believe that humans are more productive than automation.
Let's agree to disagree 😃

@douglasg14b

@nimanikoo nimanikoo requested a review from douglasg14b March 25, 2024 09:25
Copy link
Contributor

@JeremyWu917 JeremyWu917 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@douglasg14b douglasg14b left a comment

Choose a reason for hiding this comment

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

Like the cleanup :)

@TalZaccai
Copy link
Contributor

@nimanikoo thank you so much for your contribution to Garnet, I can see that there was quite a bit of work that went into this and that people are overall happy with it. We would like to address most of these via our .editorconfig rules so that we're able to enforce them in the future and that we're all on the same page with regards to styling choices. We're planning on a code-style overhaul soon, and I will definitely use your PR for reference for desired choices. I'm therefore closing it for now... Thanks again!

@TalZaccai TalZaccai closed this Mar 27, 2024
@nimanikoo nimanikoo deleted the Fixup branch March 27, 2024 05:41
@github-actions github-actions bot locked and limited conversation to collaborators May 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants