Skip to content

Expressions: Refinements to new functions#1217

Merged
olemartinorg merged 15 commits intomainfrom
chore/expr-functions-review
May 31, 2023
Merged

Expressions: Refinements to new functions#1217
olemartinorg merged 15 commits intomainfrom
chore/expr-functions-review

Conversation

@olemartinorg
Copy link
Contributor

@olemartinorg olemartinorg commented May 31, 2023

Description

I missed the review window to #1209, but it turns out I had quite a few more comments to add, so I would have just made @framitdavid's day much worse, and bothered him unnecessarily before his holiday...

Here's the changes I would have suggested:

  • Do not throw runtime errors unless we have to. I've said before that we should keep the expression language strict, and I still think that it should for comparisons, etc - but we should avoid runtime errors as much as possible. When a runtime error occurs (potentially deep inside a function), the entire expression fails, and a default value is returned. Since the language has no error handling (yet), we force developers to make extremely complex expressions to check if every value is null to make sure no runtime errors occcur. We can be sure that a null never ends with the given string, so these functions (such as endsWith) should just return the expected boolean instead of throwing an exception that bubbles up the stack.
  • Adding support for these functions to JsonSchema. To make sure we don't miss that for future built-in functions, I added a unit test to check the schema and validate against it.
  • Making the last parameter for the round function optional (and implementing support for that in the validator), so that we don't have to pass null when you want to round to the nearest integer.
  • Returning nb as the default language if the profile does not have any information. That's the default in our apps as well, and null as the default here is not really useful.
  • Fixing autocompletion for function names in vscode by using anyOf instead of oneOf

Related Issue(s)

Verification/QA

  • Manual functionality testing
    • I have tested these changes manually
    • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added/updated
    • Cypress E2E test(s) have been added/updated
    • No automatic tests are needed here (no functional changes/additions)
    • I want someone to help me make some tests
  • UU/WCAG (follow these guidelines until we have our own)
    • I have tested with a screen reader/keyboard navigation/automated wcag validator
    • No testing done/necessary (no DOM/visual changes)
    • I want someone to help me perform accessibility testing
  • User documentation @ altinn-studio-docs
    • Has been added/updated
    • No functionality has been changed/added, so no documentation is needed
    • I will do that later/have created an issue
  • Changes/additions to component properties
    • Changes are reflected in both src/layout/layout.d.ts and layout.schema.v1.json, and these are all backwards-compatible
    • No changes made
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
    • This change/feature does not require any changes to Altinn Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
    • I don't have permissions to do that, please help me out
  • Labels
    • I have added a kind/* label to this PR for proper release notes grouping
    • I don't have permissions to add labels, please help me out

Ole Martin Handeland added 13 commits May 31, 2023 09:26
# Conflicts:
#	src/layout/Group/DisplayGroupContainer.tsx
…to disable (they don't work quite right in app-frontend yet)
…pt when absolutely necessary, and trying to ["upperCase", ["dataModel", "something.without.a.value.currently"]] should not spam runtime exceptions that cause the entire expression to fail. Adding more test cases, and converting the "round" function to return a string (as it can't reliably produce the number of decimals without actually returning a string - luckily strings can still be used for further calculations, but number types are not guaranteed to actually display the number of decimal places supplied to "round").
…ync with the expression function definitions
…ns. It is impossible to match multiple schemas anyway, since all of them have to start with the unique function name.
…a arguments, as auto-completion now works well with anyOf instead of oneOf
@olemartinorg olemartinorg added the kind/bug Something isn't working label May 31, 2023
@olemartinorg olemartinorg self-assigned this May 31, 2023
@olemartinorg olemartinorg requested a review from bjosttveit May 31, 2023 10:56
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

93.3% 93.3% Coverage
0.0% 0.0% Duplication

Copy link
Member

@bjosttveit bjosttveit left a comment

Choose a reason for hiding this comment

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

Great stuff 🥇

@olemartinorg olemartinorg merged commit ab3c9b8 into main May 31, 2023
@olemartinorg olemartinorg deleted the chore/expr-functions-review branch May 31, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants