Skip to content

Correct IsWarnEnabled property in README#5

Merged
jimm98y merged 3 commits intojimm98y:mainfrom
winscripter:readme-fix
Feb 22, 2026
Merged

Correct IsWarnEnabled property in README#5
jimm98y merged 3 commits intojimm98y:mainfrom
winscripter:readme-fix

Conversation

@winscripter
Copy link
Copy Markdown
Contributor

Hi, I noticed a small discrepancy in the README example.

  1. The IMp4Logger interface doesn't have WarnEnabled - it's IsWarnEnabled instead.
  2. Properties like IsWarnEnabled or IsErrorEnabled are actually get-only. They don't have a set accessor. I can make a new PR for them to support the set accessor if you'd like

This PR fixes the former.

I would also like to make new PRs for performance and code quality improvements, if you’d be open to it.

@jimm98y
Copy link
Copy Markdown
Owner

jimm98y commented Feb 22, 2026

I haven't got time to test it yet. I'll have to update the APIs in my other projects using it as well. Yes, the accessor should be added - can you please add it to this PR as well?

@winscripter winscripter marked this pull request as draft February 22, 2026 11:13
@winscripter winscripter marked this pull request as ready for review February 22, 2026 11:49
@winscripter
Copy link
Copy Markdown
Contributor Author

@jimm98y Done, set accessor has been added.

@winscripter
Copy link
Copy Markdown
Contributor Author

winscripter commented Feb 22, 2026

I would also like to submit PRs to improve performance as well as code quality to use latest C# language features, given that LangVersion is set to latest. Would you like me to work on refactoring and performance improvements on a new PR?

@jimm98y
Copy link
Copy Markdown
Owner

jimm98y commented Feb 22, 2026

Latest C# features are certainly nice, but in my opinion they don't always improve code quality and they make the code less portable and sometimes harder to understand. If it would provide a meaningful performance bump then sure, I'd say go for it and we can discuss it over some performance benchmarks. Bear in mind this project is still targeting netstandard2.0 so not all latest features will work without polyfills.

@jimm98y jimm98y merged commit 4dfeb9d into jimm98y:main Feb 22, 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.

2 participants