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

Features zerowarnings: Changes which alter code and could affect functionality in the base library #4882

Conversation

workgroupengineering
Copy link
Contributor

What does the pull request do?

Split PR #4499
#4499 (comment)

maxkatz6
maxkatz6 previously approved these changes Oct 24, 2020
Copy link
Member

@maxkatz6 maxkatz6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
I don't see any dangerous changes.

@workgroupengineering
Copy link
Contributor Author

hi @maxkatz6, i synchronized the PR with the latest commit of the master branch. When you have time you can review the PR again.

@maxkatz6
Copy link
Member

@workgroupengineering we discussed this PR before, and we think it should be merged after 0.10 release.

@workgroupengineering
Copy link
Contributor Author

@workgroupengineering we discussed this PR before, and we think it should be merged after 0.10 release.

Thanks for the reply.
I was hoping this PR was included in version 0.10, but that's okay.

hi @maxkatz6, i synchronized the PR with the latest commit of the master branch. When you have time you can review the PR again.

I asked this question to understand if I should continue to maintain this PR or should I abandon it.

…ullable reference types should only be used in code within a '#nullable'
… doesn't match overridden member (possibly because of nullability attributes)
…n type of 'ICommand? MenuItem.Command.get' doesn't match implicitly implemented member 'ICommand ICommandSource.Command.get' (possibly because of nullability attributes).
…ulture=neutral, PublicKeyToken=null' does not have a strong name.
@workgroupengineering
Copy link
Contributor Author

I have updated the PR, there are only 16 alerts.

14 of which are related to IME support for X11, I have not suppressed them as I don't know the implementation is complete.

The other two are related to the creation of the nuget package on macOS and I have not yet understood what it depends on.

Here C.I. Build.

@pr8x
Copy link
Collaborator

pr8x commented Jan 27, 2021

Would be nice to get this in soonish! CC: @grokys

@grokys
Copy link
Member

grokys commented Jan 27, 2021

Definitely, I plan to work on this soon. Looking at the changes, I feel there are still low impact changes and high-impact changes mixed together so I think I need to go through the PR and either separate the two types of changes out or do a very thorough review.

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@workgroupengineering I started reviewing this but then I figured that it'll just be easier to cherry pick the parts of this PR that are correct and fix the parts that I'm confident of the correct fix. I will open a new PR and add you as a co-author to the commits.

The PR probably won't fix all the issues (because as you've seen, it's difficult to fix some warnings if you don't know the code well) so I will leave this PR open to track remaining warnings. Thanks for all your work here!

src/Avalonia.Base/ValueStore.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/Data/BindingValue.cs Show resolved Hide resolved
src/Avalonia.Controls/Application.cs Show resolved Hide resolved
grokys added a commit that referenced this pull request Feb 5, 2021
Co-Authored-By: workgroupengineering <workgroupengineering@users.noreply.github.com>
@grokys grokys mentioned this pull request Feb 5, 2021
@workgroupengineering
Copy link
Contributor Author

I updated the PR according to your instructions.
I did it thinking it might be useful to you.

@maxkatz6 maxkatz6 dismissed their stale review February 5, 2021 19:23

Need to review it again.

@workgroupengineering workgroupengineering deleted the features/zerowarnings_core branch November 23, 2021 08:19
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.

None yet

5 participants