-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add IProgressListener #5139
Add IProgressListener #5139
Conversation
@@ -21,7 +21,7 @@ | |||
using SonarLint.VisualStudio.SLCore.Core; | |||
using SonarLint.VisualStudio.TestInfrastructure; | |||
|
|||
namespace SonarLint.VisualStudio.SLCore.UnitTests | |||
namespace SonarLint.VisualStudio.SLCore.UnitTests.Core |
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.
moved tests to core folder so it would have same structure with the main project. It's 2 tasks in 1 PR but since PR is so small I think it's OK.
@@ -22,7 +22,7 @@ | |||
using SonarLint.VisualStudio.SLCore.Core; | |||
using SonarLint.VisualStudio.TestInfrastructure; | |||
|
|||
namespace SonarLint.VisualStudio.SLCore.UnitTests; | |||
namespace SonarLint.VisualStudio.SLCore.UnitTests.Core; |
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.
see comment on SLCoreListenerSetUpTests. (This also had some whitespace fixes done automatically)
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.
Give me some time to make the parameter deserialization work without annotations. Meanwhile you can remove the interface as it is not needed and change the export contract.
{ | ||
public interface IProgressListener : ISLCoreListener | ||
{ | ||
[JsonRpcMethod(UseSingleObjectParameterDeserialization = true)] |
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.
- I don't know if this is going to work when you put it on the interface instead of putting it on the class. 2. We can set this property in JsonRpcTargetOptions. It didn't work for me in the PoC, but I have a suspicion I did something wrong
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.
I will try to make the PoC work. Will get back to you.
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.
Discussed on slack
|
||
namespace SonarLint.VisualStudio.SLCore.Listener | ||
{ | ||
public interface IProgressListener : ISLCoreListener |
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.
We don't need this interface.
[PartCreationPolicy(CreationPolicy.Shared)] | ||
public class ProgressListener : IProgressListener | ||
{ | ||
public Task StartProgress(object parameters) |
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.
You need to add a comment with why this is an object and not some real model ("we don't support this method")
Task ReportProgress(object parameters); | ||
} | ||
|
||
[Export(typeof(IProgressListener))] |
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.
Wrong export contract
[TestMethod] | ||
public void MefCtor_CheckIsExported() | ||
{ | ||
MefTestHelpers.CheckTypeCanBeImported<ProgressListener, IProgressListener>(); |
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.
Wrong export contract
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
50ff0a1
into
feature/sloop-rule-meta-data
Fixes #5134