- Notifications
You must be signed in to change notification settings - Fork778
OnUIThread-Refactor and a few improvements#963
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…gedBase.cs) to remove code duplication.- Added documentation in (RegExHelper.cs) for NameRegEx (Hover over NameRegEx to see the result).- Changed action variables to Local functions.- Changed nested private classes to be sealed.- Inlined the out parameters.- Used Pattern matching whenever applicable.- Used Compound assignment whenever applicable.- Used nameof to avoid magic strings.- Removed this Keyword whenever possible.- Removed Zombie code.- Removed unnecessary using directives.- Remoced #region preprocessor directives.- Reformat Curly Braces- Fixed couple of typos.
vb2ae left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for this. Thank you for fixing the typos in the comments.
Uh oh!
There was an error while loading.Please reload this page.
KasperSK left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall it looks good, I have just two minor changes.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
- Changed all usage of 'SafeRun' to use the new location of the method.- Removed the "'s" from the documentation of DebugLog class.- Changed the local variable name from (namespaceEncoded) to 'encodedNamespace'.
K4PS3 commentedJan 24, 2025
@vb2ae@KasperSK
@KasperSK Changing the parameter name from 'srcNamespace' to 'sourceNamespace' will be considered 'Breaking change' and maybe affect someone using it, because it's the public API surface. |
KasperSK commentedJan 24, 2025
Regarding the We can do that name change since the current major version is 4 and we are going to release this with version 5. The reason I brought it up was that it felt mismatched to have one name use shorthand notation and not the other. |
vb2ae left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SafeRun makes the code look cleaner. Documentation is always good to have
KasperSK commentedFeb 12, 2025
Yeah, but I still think we should rename it having a public Method called SafeRun implies that nothing can go wrong and I don't think that is the case here as all it does is dispatch to the UI thread if the platform requires it. |
Uh oh!
There was an error while loading.Please reload this page.
- Added Documentation to implies the fact that action could be failed, safety not guaranteed.
This pull request to improve parts of the codebase for better readability and to fix the compiler warnings.
I think this pull request should be easy to review since the changes in the files are at minimum.
The code in "Platforms" can be improved, but will need extra work to be done.
You can take notes from this pull request without merging it, I don't mind that.
Summary of changes:
SafeRun in Action
Regex Documentation