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

OnUIThread-Refactor and a few improvements #963

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

K4PS3
Copy link

@K4PS3 K4PS3 commented Jan 14, 2025

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

SafeRun

Regex Documentation

regex

…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.
Copy link
Member

@vb2ae vb2ae left a 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.

Copy link
Member

@KasperSK KasperSK left a 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.

src/Caliburn.Micro.Core/DebugLog.cs Show resolved Hide resolved
src/Caliburn.Micro.Platform.Core/RegExHelper.cs Outdated Show resolved Hide resolved
- 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
Copy link
Author

K4PS3 commented Jan 24, 2025

@vb2ae @KasperSK
I just pushed another commit with these changes:

  • Moved 'SafeRun' to 'Execute' Helper class.
  • 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'.

@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.
Ex. any call with argument name like this will break:
RegExHelper.NamespaceToRegEx(srcNamespace: nsSource + ".")
Are we good to go with this change?

@K4PS3 K4PS3 requested review from KasperSK and vb2ae January 24, 2025 05:59
@KasperSK
Copy link
Member

@vb2ae @KasperSK I just pushed another commit with these changes:

  • Moved 'SafeRun' to 'Execute' Helper class.
  • 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'.

@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. Ex. any call with argument name like this will break: RegExHelper.NamespaceToRegEx(srcNamespace: nsSource + ".") Are we good to go with this change?

Regarding the SafeRun method could we give it a more descriptive name? To me safe indicates that nothing can go wrong (Exceptions) so I think a more fitting name would be DispatchIfRequired or something along those lines.

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.

Copy link
Member

@vb2ae vb2ae left a 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
Copy link
Member

SafeRun makes the code look cleaner. Documentation is always good to have

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.

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.

3 participants