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

Lionel Smith-Gordon Use of Asset in AssetCriteria #3228

Conversation

regnosys-prod-user
Copy link
Collaborator

No description provided.

Change from AssetIdentifier to Asset itself
Copy link

netlify bot commented Nov 7, 2024

Deploy Preview for finos-cdm ready!

Name Link
🔨 Latest commit 7dabda8
🔍 Latest deploy log https://app.netlify.com/sites/finos-cdm/deploys/67322d7c2e7b3400086dd81a
😎 Deploy Preview https://deploy-preview-3228--finos-cdm.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Oblongs
Copy link
Contributor

Oblongs commented Nov 7, 2024

Reviewed in the Collateral Working Group on 29 October 2024, see #3169.

chrisisla
chrisisla previously approved these changes Nov 8, 2024
@@ -220,13 +220,13 @@ type AssetCriteria:
agencyRating AgencyRatingCriteria (0..*)
maturityType MaturityTypeEnum (0..1)
maturityRange PeriodRange (0..1)
assetIdentifier AssetIdentifier (0..*)
specificAssets Asset (0..*)
Copy link
Contributor

Choose a reason for hiding this comment

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

The design principles impose a singular naming convention even when we are expecting lists of data i.e. specificAsset should be used here. I think adding the 's' on the end makes more sense in this instance.

Copy link
Contributor

@valdensmith valdensmith left a comment

Choose a reason for hiding this comment

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

Line 248, in the description, there is a typo on the word loan

Copy link
Contributor

@valdensmith valdensmith left a comment

Choose a reason for hiding this comment

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

Same description is it also worth adding the details that this attribute can be used for specific source ids such as ISINS/ CUSIPS

Copy link
Contributor

@valdensmith valdensmith left a comment

Choose a reason for hiding this comment

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

Everything else looks good as we discussed

valdensmith
valdensmith previously approved these changes Nov 8, 2024
@Oblongs Oblongs dismissed stale reviews from valdensmith and chrisisla via d12c999 November 8, 2024 10:18
@Oblongs
Copy link
Contributor

Oblongs commented Nov 8, 2024

Same description is it also worth adding the details that this attribute can be used for specific source ids such as ISINS/ CUSIPS

Typo fixed and description enhanced

@valdensmith
Copy link
Contributor

valdensmith commented Nov 8, 2024 via email

valdensmith
valdensmith previously approved these changes Nov 8, 2024
@hugohills-regnosys hugohills-regnosys merged commit 4e0a19c into finos:master Nov 11, 2024
7 checks passed
@hugohills-regnosys hugohills-regnosys deleted the lionel_auth0_65080e6e10b98cc73ac44ca1-AssetCriteria branch November 11, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants