Open dahlbyk opened 1 year ago
@dahlbyk Thanks for creating this PR and raising the questions. Let me review them over the weekend and get back to you as I do have any local setup for the SDK anymore. If something breaks for logging apart from serilog deprecation then we will be in a messy situation. @MatthewSteeples would you be able to review this PR and provide some guidance or if you already have a solution for this in your fork?
cc: @sujitharamadass
I've rebased this branch off the latest master and included a few other changes. We're validating it in our test environment for the next couple of weeks, and then planning on our production environment after that.
@MatthewSteeples which of the existing logging features are you using?
I'm wondering if there's value in this intermediate step, vs just one big breaking change that completely revamps logging with the modern abstraction.
We've not been using any of the built in logging features really. We did a while ago but for the most part if we're having issues we end up resorting to Fiddler and the Debugger (or enabling tracing in the framework to log the requests and responses).
Happy to talk through making it one big change. Does make more sense than having a few breaking changes one after the other. I did try some of that on a separate branch but it was a pretty big rabbit hole to go down. I was replacing centralised logging with logging per class, and trying to wire in dependency injection at the same time though, so possibly bit off too much
…but it was a pretty big rabbit hole to go down. I was replacing centralised logging with logging per class, and trying to wire in dependency injection at the same time though, so possibly bit off too much
With just this you've convinced me we need intermediate steps. 😁 Consumers deserve deprecations notices anyway. Roughly:
@MatthewSteeples thanks for working on removing the Serilog dependency. Could you help me understand how far along you are with these changes?
@MatthewSteeples @dahlbyk Can you please share the status of this PR?
Apologies, Intuit's deprecation of Desktop in the UK has taken more of my time than expected (as my team is responsible for migrating the customers to Quickbooks Online). I started organising some time with dahlbyk to go over this PR and the general plan but never followed up on it. I hope to be able to start looking into it again towards the end of July
Thanks @MatthewSteeples . Let us know if there is any update on this.
@dahlbyk @MatthewSteeples Any update on this PR?
Did he not clarify he meant end of July 2024? 😏
I'm still interested in landing this PR, and https://github.com/intuit/QuickBooks-V3-DotNET-SDK/pull/221 before it (if you care at all about a deprecation cycle). If @MatthewSteeples still has limited availability I could be contracted to take the lead on modernizing this SDK.
Apologies all, had a lot going on. Still very much interested in getting this library modernised. I've dropped you an email @dahlbyk
For a more modern library for the QBO API, consider trying QuickBooksSharp. It is a modern version that uses async, modern .NET and has minimal dependencies.
Disclaimer: I created this library because I had so many issues working with the official .NET library. While some functionality might still be missing compare to the official API, I'm confident that the dev experience is much better. Contributions from the community are welcome.
Thanks @MatthewSteeples @dahlbyk . Will wait for your update.
Did he not clarify he meant end of July 2024? 😏
I'm still interested in landing this PR, and #221 before it (if you care at all about a deprecation cycle). If @MatthewSteeples still has limited availability I could be contracted to take the lead on modernizing this SDK.
Is there any assistance I could provide towards landing this PR and #221 ?
Opening as a draft so we can evaluate the end state after Serilog is removed. A few observations:
It strikes me as maybe not ideal that we end up with an extra level of indirection that ultimately provides no value:
ippConfig.Logger.CustomLogger
could beippConfig.Logger
ippConfig.AdvancedLogger.Logger
could beippConfig.AdvancedLogger
Not sure there's an alternative in the short-term, so maybe leave like that until #286?
Inuit.Ipp.Core
ends up defaulting toTraceLogger
in all cases, which seems reasonable, butOAuth2PlatformClient
defaults toNullOAuthLogger
. Maybe aTraceOAuthLogger
would be a better default?IOAuthLogger
andNullOAuthLogger
end up being the only types inIntuit.Ipp.OAuth2PlatformClient.Diagnostics
. Does that need to be a separate project? Maybe I should try to find a way to avoid addingIOAuthLogger
so the project can just go away?IAdvancedLogger
andIOAuthLogger
do a bit of basic logging, but mostly exist to log request and response. I designedIOAuthLogger
for this, butIAdvancedLogger
is a simpleLog()
. Should they be more similar? ShouldLogRequest()
/LogResponse()
be extension methods onIntuit.Ipp.Diagnostics.ILogger
(orMicrosoft.Extensions.Logging.ILogger
, sinceOAuth2PlatformClient
doesn't depend onIntuit.Ipp.Diagnostics
)?