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

DOPE-226: added array functions, CM extension functions and tests #34

Merged
merged 5 commits into from
Jul 11, 2024

Conversation

martinagallati-ergon
Copy link
Collaborator

Info for ARRAY_PREPEND: In the documentation for the function, the values precede the array expression. To implement the variable number of arguments I used vararg, which would impose a need to use a named argument for array. For usability's sake I did it analogous to ARRAY_APPEND(array, vararg values), so a DOPE user does not need to name the array argument.

Copy link
Collaborator

@m0ne m0ne left a comment

Choose a reason for hiding this comment

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

In general, there are a few files that exceed the optimal length, none that exceed the maximum, but I would advise against longer lines than necessary. I would go through the PR again and break up long lines. Also, I noticed that the spacing between functions is now mixed (sometimes there is a blank line, sometimes there is none), check that again.

And naming wise, there are places where we use

firstArray: TypeExpression<ArrayType>,
secondArray: TypeExpression<ArrayType>,
vararg arrays: TypeExpression<ArrayType>,

the name arrays is quite vague and it does not seem clear that we mean by arrays firstArray, secondArray, thirdArray, fourthArray,...

@martinagallati-ergon martinagallati-ergon merged commit 0a01401 into main Jul 11, 2024
1 check passed
@martinagallati-ergon martinagallati-ergon deleted the feature/dope-226-add-array-functions branch July 11, 2024 14:42
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.

4 participants