-
Notifications
You must be signed in to change notification settings - Fork 13
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
Update to 2.1 specification #141
base: main
Are you sure you want to change the base?
Conversation
… market identifiers
… market identifiers
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
==========================================
+ Coverage 76.56% 77.22% +0.66%
==========================================
Files 53 62 +9
Lines 431 527 +96
Branches 44 48 +4
==========================================
+ Hits 330 407 +77
- Misses 82 98 +16
- Partials 19 22 +3 ☔ View full report in Codecov by Sentry. |
@@ -33,7 +33,7 @@ public DesktopAgent() | |||
|
|||
public Task<IListener> AddContextListener<T>(string? contextType, ContextHandler<T> handler) where T : IContext | |||
{ | |||
return _currentChannel?.AddContextListener<T>(contextType, handler); | |||
return _currentChannel?.AddContextListener<T>(contextType, handler) ?? throw new Exception("Unable to create listener"); |
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.
This is a long existing build warning in the example project about possible null return value when the return is declared as non nullable
/// </summary> | ||
public string Name { get; set; } | ||
public string ID { get; set; } |
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.
AppChannel Name changed to ID to avoid confusion. This required changes to class and sample json used for deserialization in unit testing.
Name = name ?? throw new ArgumentNullException( nameof(name)); | ||
#pragma warning restore CS0618 // Type or member is obsolete |
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.
Even though Name is marked as obsolete/decomissioned we are still using it. In order to not have build errors in our own code about using obsolete member, disable the warning.
@@ -28,6 +28,6 @@ public class ContactID | |||
{ | |||
public string? Email { get; set; } | |||
|
|||
public string? FdsId { get; set; } | |||
public string? FDS_ID { get; set; } |
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.
This was was previously incorrect and is being fied here.
|
||
object? IContext<object>.ID => base.ID; | ||
} | ||
|
||
public class ChatInitSettingsOptions |
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.
Not sure why I was so lazy in past implementation of just taking an object. I defined it as a custom type now.
public static readonly string Nothing = "fdc3.nothing"; | ||
public static readonly string Organization = "fdc3.organization"; | ||
public static readonly string Portfolio = "fdc3.portfolio"; | ||
public static readonly string Position = "fdc3.position"; | ||
public static readonly string TimeRange = "fdc3.timerange"; | ||
public static readonly string TimeRange = "fdc3.timeRange"; | ||
public static readonly string TransactionResult = "fdc3.transactionResult"; | ||
public static readonly string Valuation = "fdc3.valuation"; | ||
|
||
public static IDictionary<string, Type> Map = new Dictionary<string, Type>() |
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.
For those wondering, this is useful for DesktopAgents to use as a mapping when doing deserialization. Based on the context type string on the message, do the lookup here to get the concrete type and then deserialize to that type. Note that this is also public so your desktopagent can add/overwrite in their implementation as they see fit such as mapping to a derived context type rather than the base default.
* and limitations under the License. | ||
*/ | ||
|
||
namespace Finos.Fdc3.Context |
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.
As discussed on standards call. This is being defined as string constants for conditional statements and creating messages as an enum would be serialized to lower case and not be correct. The default serialization for enum is to lower case based on other areas of the api and contexts so to use an enum here would require custom serialization on TransactionResult
@@ -27,6 +29,7 @@ public interface IIntentMetadata | |||
/// <summary> | |||
/// A friendly display name for the intent that should be used to render UI elements. | |||
/// </summary> | |||
string DisplayName { get; } | |||
[Obsolete("Use the intent name for display as display name may vary for each application as it is defined in the app's AppD record.")] | |||
string? DisplayName { get; } |
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.
obsolete/decommissioned and also changed to nullable
[Fact] | ||
public async Task Message_SerializedJsonMatchesSchema() | ||
{ | ||
Message message = new Message(new MessageText() { TextPlain = "textplain", TextMarkdown = "textmarkdown" }); |
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.
This is an example of how we defined the type with TextPlain/TextMarkdown string properties and the schema validation below will pass if they are serialized to text/plan and text/markdown
Any desire if you are already there changing the tests to be async Task instead of async void? |
They were changed from async void to async Task to cleanup warnings in preparation for next xunit release. |
this.Message = message; | ||
} | ||
|
||
public string Status { get; set; } |
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 TransactionResultStatus on this not being an enum but just a string
public MessageText Text { get; set; } | ||
} | ||
|
||
public class MessageText |
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.
Custom serializer provided in NewtonsoftJson or Json packages correctly round-trip these by changing to text/plan and text/markdown
{ | ||
public class Message : Context, IContext | ||
{ | ||
public Message(MessageText text, object? id = null, string? name = null) |
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.
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.
Great catch, will update.
{ | ||
} | ||
|
||
[Fact] | ||
public async void Organization_SerializedJsonMatchesSchema() | ||
public async Task Organization_SerializedJsonMatchesSchema() | ||
{ | ||
Organization organization = new Organization(new OrganizationID() { FDS_ID = "fdc_id", LEI = "lei", PERMID = "permid" }, "organization"); |
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.
Very minor (and outside the changes, but since I noticed), it seems to be "fdcsid" everywhere else, so I was going to suggest changing for consistency
Organization organization = new Organization(new OrganizationID() { FDS_ID = "fdc_id", LEI = "lei", PERMID = "permid" }, "organization"); | |
Organization organization = new Organization(new OrganizationID() { FDS_ID = "fdsid", LEI = "lei", PERMID = "permid" }, "organization"); |
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.
Looks like maybe a copy/paste and not changing all mock test data. I'll go through and map the mock data to the field name for all instances of this field.
Update to 2.1. Checklist and work detailed in #74
THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE FINOS CORPORATE CONTRIBUTOR LICENSE AGREEMENT.
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.