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

refactor: Use new collection initializers #292

Merged
merged 15 commits into from
Dec 23, 2023

Conversation

AlmarAubel
Copy link
Contributor

I've made some updates in our codebase to leverage the new list initializer syntax introduced in C# 12. This change aims to enhance readability and maintainability of our code.

However, I encountered an issue with StyleCop. Currently, StyleCop has a known problem (#3745, #3687) that triggers SA01010 as a false positive when using this new syntax. Until this is resolved in StyleCop, we might see this warning.

I'd appreciate your feedback on these changes. Please share your thoughts if you like this kind of pull request And if you're okay with the interim StyleCop warning we'll encounter.

@AlmarAubel AlmarAubel force-pushed the refactor-collection-initializers branch from fd2bc45 to 5f2ea75 Compare December 15, 2023 09:04
@bistok
Copy link
Contributor

bistok commented Dec 15, 2023

I was tempted to do this change when migrated to .NET 8, but not do it because the Stylecop warnings, I see they merged today the commit that fixed this and marked it to the next release, I think we can live some time because they will fix it when releasing the next package version.

@kgrzybek
Copy link
Owner

@AlmarAubel

I really like this type of pull request.

However, the problem lies with these warnings. I want to treat this project to be as close to the production implementation as possible. In projects I participate in, we treat all warnings as errors, and I consider it a good practice.

Therefore, I would prefer to wait with merging this pull request until a new version of StyleCop is released. WDYT?

@AlmarAubel
Copy link
Contributor Author

Yes totally agree, at my professional projects we also treat warnings as errors. So let's wait. I will monitor the release of the new version of Stylecop and will add the version bump to this pr once released.

@kgrzybek
Copy link
Owner

Good, let's wait.

Meanwhile, I removed all warnings and enabled "warnings as errors" in fed5f5f

@AlmarAubel
Copy link
Contributor Author

I've resolved the issue using Stylecop and updated the package accordingly. Apologies for the additional commits resulting from an error while merging from master. I hope you can squash merge this commit to streamline the changes.

@kgrzybek
Copy link
Owner

Good job @AlmarAubel, thanks for the active monitoring of StyleCop repo!

@kgrzybek kgrzybek merged commit f04bd59 into kgrzybek:master Dec 23, 2023
2 checks passed
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.

None yet

3 participants