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

[patch] move cell value type to a new attribute "cellValueType" in custom function metadata #867

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

xai-msft
Copy link
Contributor

@xai-msft xai-msft commented Jun 7, 2024

Cell value types have been introduced to custom function input parameter type. We decided to move it to a new attribute "cellValueType" for backward compatibility, so that older client won't break on unknown types.
"cellValueType" works as follows:
When "type" is one of "boolean"/"number"/"string", "cellValueType" won't be parsed by Excel. When "type" is "any" and "cellValueType" is a valid cell value type, the cell value type will be used as custom function parameter type. If "cellValueType" is invalid, Excel will use the fallback "type" "any".

Sample function metadata before change:

{
"description": "Test Excel.CellValue parameter",
"id": "TESTCELLVALUEPARAM",
"name": "TESTCELLVALUEPARAM",
"parameters": [
{
"name": "x",
"type": "cellvalue"
}
],
"result": {}
}

after change:

{
"description": "Test Excel.CellValue parameter",
"id": "TESTCELLVALUEPARAM",
"name": "TESTCELLVALUEPARAM",
"parameters": [
{
"name": "x",
"type": "any"
"cellValueType": "cellvalue"
}
],
"result": {}
}

  1. Do these changes impact command syntax of any of the packages? (e.g., add/remove command, add/remove a command parameter, or update required parameters)
    No

  2. Do these changes impact documentation? (e.g., a tutorial on https://learn.microsoft.com/office/dev/add-ins/overview/office-add-ins)
    Cell value types are not public yet.

Validation/testing performed:
Updated and ran tests.

@xai-msft xai-msft requested a review from a team as a code owner June 7, 2024 08:02
Copy link
Contributor

@akrantz akrantz left a comment

Choose a reason for hiding this comment

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

The change is simple but I'd like to discuss more about the naming here. I feel like there should be more examples / test cases. This is not really about the change here is what metadata is generated, but really is more about the native code which is parsing and handling the metadata. I wish I had known about this discussion earlier.

Copy link

@AlexJerabek AlexJerabek left a comment

Choose a reason for hiding this comment

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

Corollary to this: aren't there doc changes that'll need to happen? Have those been requested from the content team?

packages/custom-functions-metadata/src/parseTree.ts Outdated Show resolved Hide resolved
@xai-msft xai-msft changed the title [patch] move cell value type to subtype in custom function metadata [patch] move cell value type to a new attribute "cellValueType" in custom function metadata Jun 18, 2024
@xai-msft
Copy link
Contributor Author

Hi @akrantz , I have updated the change according to our discussions. Could you please help merge this as I don't have permission? Thank you!

akrantz
akrantz previously approved these changes Jun 20, 2024
Copy link
Contributor

@akrantz akrantz left a comment

Choose a reason for hiding this comment

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

Looks good!

@akrantz akrantz requested a review from AlexJerabek June 20, 2024 14:55
@millerds
Copy link
Contributor

/azurepipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@millerds
Copy link
Contributor

@xai-msft . . . Alex's question still needs to be addressed.

@AlexJerabek
Copy link

@millerds @xai-msft @akrantz
The PR description says these changes aren't public yet. Do we have an idea when they will be? Is there a preview phase? As long as this PR is separate from the feature request, we can loop in @alison-mk separately to document the new cellValueType and behavioral changes to type.

@millerds
Copy link
Contributor

@millerds @xai-msft @akrantz The PR description says these changes aren't public yet. Do we have an idea when they will be? Is there a preview phase? As long as this PR is separate from the feature request, we can loop in @alison-mk separately to document the new cellValueType and behavioral changes to type.

This repo does not have separate internal or preview releases branches (we haven't needed one yet). Once this gets checked in it will be part of the next publish of the packages (which is not on a set time schedule) . . . making it at least preview even if the full support of it is not official yet.

@AlexJerabek
Copy link

Thanks @millerds! Then I'd like to wait to check this in until we have a minimum set of documentation ready to go in concurrently. We have a legal requirement to document any public API surface area, so we'll at least need some mention of the new attribute.

@xai-msft and @alison-mk can get that staged in the https://github.com/OfficeDev/office-js-docs-pr repo.

@alison-mk
Copy link

Thanks, all! I'll reach out to @xai-msft internally to coordinate the docs updates. I started a draft PR here: OfficeDev/office-js-docs-pr#4632. Agreed that we should align the docs update with the feature release if possible.

@xai-msft
Copy link
Contributor Author

Thanks @millerds for the reminder and thank you all for the discussions. I'll work with Alison on documentations and update here when it's ready.

@OfficeDev OfficeDev deleted a comment from MiaofeiWang Jul 22, 2024
"doublecellvalue": "number",
"entitycellvalue": "any",
"errorcellvalue": "any",
"formattednumbercellvalue": "any",
Copy link
Contributor

@akrantz akrantz Aug 28, 2024

Choose a reason for hiding this comment

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

Should formattednumbercellvalue map to number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that makes sense.

akrantz
akrantz previously approved these changes Aug 28, 2024
Copy link
Contributor

@akrantz akrantz left a comment

Choose a reason for hiding this comment

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

Made a suggestion about formattednumbercellvalue mapping but looks good to me otherwise. I've actually happen to see this updated to cellValueType and appreciate being receptive to previous feedback that I have given.

@akrantz
Copy link
Contributor

akrantz commented Aug 29, 2024

@AlexJerabek We should go ahead with this change before documentation has been updated. It won't have any impact on what a user would do. This is just to make improvements so Excel know more about the cell value types so it can coerce types to the desired cell value type.

@akrantz akrantz removed the request for review from AlexJerabek August 29, 2024 14:06
@MiaofeiWang
Copy link

It seems merging this PR still needs repo admin's help. Would you please help run the status check, @akrantz @millerds ? Thank you.

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.

6 participants