Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
K4PS3 wants to merge3 commits intoCaliburn-Micro:master
base:master
Choose a base branch
Loading
fromK4PS3:OnUIThread-Refactor

Conversation

@K4PS3
Copy link

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

@vb2aevb2ae 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

@KasperSKKasperSK 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.

- 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

@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?

@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 theSafeRun 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 beDispatchIfRequired 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

@vb2aevb2ae 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.

- Added Documentation to implies the fact that action could be failed, safety not guaranteed.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@KasperSKKasperSKKasperSK requested changes

@vb2aevb2aevb2ae approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@K4PS3@KasperSK@vb2ae

[8]ページ先頭

©2009-2025 Movatter.jp