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

GH-3069 use of Logger with source code generator #3429

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lillo42
Copy link
Contributor

@lillo42 lillo42 commented Dec 20, 2024

I'm starting with one class, so we can validate that I'm going with a good path or if we should change it

Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

Code Health Quality Gates: OK

Change in average Code Health of affected files: +0.26 (8.28 -> 8.55)

  • Improving Code Health: 1 findings(s) ✅
  • Affected Hotspots: 1 files(s) 🔥

View detailed results in CodeScene

@@ -48,7 +48,7 @@ namespace Paramore.Brighter
/// Implements both the <a href="http://www.hillside.net/plop/plop2001/accepted_submissions/PLoP2001/bdupireandebfernandez0/PLoP2001_bdupireandebfernandez0_1.pdf">Command Dispatcher</a>
/// and <a href="http://wiki.hsr.ch/APF/files/CommandProcessor.pdf">Command Processor</a> Design Patterns
/// </summary>
public class CommandProcessor : IAmACommandProcessor
public partial class CommandProcessor : IAmACommandProcessor

Choose a reason for hiding this comment

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

✅ Getting better: Code Duplication
reduced similar code in: DepositPost,DepositPostAsync

@lillo42
Copy link
Contributor Author

lillo42 commented Dec 22, 2024

Do you prefer folks, one big PR to change everything or multi-PRs (one per project)?

@iancooper
Copy link
Member

@lillo42 Thanks for picking this up. Probably a lot of work. With this kind of change, I tend to suggest that we work as follows:

  • Add an ADR to explain what you are doing. Folks who review can read that and sample
  • Just target V10.
  • Do a big PR. We can merge and fix as we verify V10. I doubt anyone will successfully trawl every statement that you write.

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.

2 participants