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

Update standardized-functions.md #3232

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vallslaura
Copy link
Contributor

@vallslaura vallslaura commented Nov 8, 2024

Updated documentation, @PayalKhanna, solved issues.

@vallslaura vallslaura requested a review from a team as a code owner November 8, 2024 07:36
Copy link

netlify bot commented Nov 8, 2024

Deploy Preview for finos-cdm ready!

Name Link
🔨 Latest commit 1fedd73
🔍 Latest deploy log https://app.netlify.com/sites/finos-cdm/deploys/672dbf6c79c9210008bb5feb
😎 Deploy Preview https://deploy-preview-3232--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.

Copy link
Contributor

@chrisisla chrisisla left a comment

Choose a reason for hiding this comment

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

Nice piece of documentation 👍

inputs:
trade Trade (1..1)
output: standardizedSchedule StandardizedSchedule (1..1)
alias assetClass:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion, but by including the entire function in the documentation you are obligating yourself to update the documentation whenever the function changes. Where functions have been described in similar documents we tend to just put the function name, inputs and output to prevent this maintenance overhead.

assetClass StandardizedScheduleAssetClassEnum (1..1)
durationInYears number (1..1)
output: percentage number (1..1)
set percentage:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as previous comment, it's nice to see the logic but really users should go into a modelling tool to review it

I won't add this comment to the other functions but its the same thing

@PayalKhanna
Copy link
Contributor

@vallslaura build is still failing

@vallslaura
Copy link
Contributor Author

@PayalKhanna All checks have passed now, build failure has been solved.

@PayalKhanna
Copy link
Contributor

All good thanks

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.

3 participants