Skip to content

[Fix_#873] Rename output.from and output.to #892

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

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

fjtirado
Copy link
Collaborator

@fjtirado fjtirado commented Jun 7, 2024

Fix #873
output.from is renamed as output.as
output.to is renamed as export.as

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Use Cases
  • Community
  • CTK
  • Other

Discussion or Issue link:

What this PR does:

Additional information:

output.from is renamed as output.as
output.to is renamed as export.as

Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
@fjtirado fjtirado force-pushed the Fix_#873 branch 3 times, most recently from 40661e1 to 0d0357c Compare June 7, 2024 11:24
@fjtirado fjtirado requested a review from cdavernas June 7, 2024 11:25
Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

Don't you feel export/to would be better than export/as, knowing it actuals transforms the context data?

@JBBianchi @ricardozanini ?

Other than that, I'd rephrase some descriptions to make it more agnostic and a bit less technical.

@JBBianchi
Copy link
Member

JBBianchi commented Jun 7, 2024

Don't you feel export/to would be better than export/as, knowing it actuals transforms the context data?

For me to implies a destination whereas as is more about the shape/format. For instance, "export to file" or "export as PDF". But I don't really have a strong opinion on the matter, both work.

@cdavernas
Copy link
Member

Allright! @fjtirado ?

@ricardozanini
Copy link
Member

@cdavernas @JBBianchi @fjtirado given the context, I also believe export/to has a stronger impact here since we are modifying the context. But again, a slight difference that in the end, both might work IMO.

@fjtirado fjtirado marked this pull request as ready for review June 7, 2024 13:27
@fjtirado fjtirado requested a review from cdavernas June 7, 2024 13:27
@cdavernas
Copy link
Member

cdavernas commented Jun 7, 2024

2v2 draw for export/as vs export/to. @matthias-pichler-warrify, care to tie break?

@matthias-pichler
Copy link
Collaborator

2v2 draw for export/as vs output/to. @matthias-pichler-warrify, care to tie break?

I'll quickly catch up on the discussion and then get back to you

@fjtirado fjtirado force-pushed the Fix_#873 branch 3 times, most recently from 01a2d7e to b4b7875 Compare June 7, 2024 13:46
@matthias-pichler
Copy link
Collaborator

2v2 draw for export/as vs export/to. @matthias-pichler-warrify, care to tie break?

Ok so I have no strong opinions here. I agree with

For me to implies a destination whereas as is more about the shape/format. For instance, "export to file" or "export as PDF". But I don't really have a strong opinion on the matter, both work.

that as sounds better in this case. Especially having used AWS StepFunctions a lot which uses a ResultPath so I initially thought that output/to also gave a destination.

therefore I think

export:
  schema: {}
  as: "..."
output:
  schema: {}
  as: "..."

is my (only slightly) preferred option

| Property | Type | Required | Description |
|----------|:----:|:--------:|-------------|
| schema | [`schema`](#schema) | `no` | The [`schema`](#schema) used to describe and validate context.<br>*Included to handle the non frequent case in which the context has a known format.* |
| as | `string`<br>`object` | `no` | A runtime expression, if any, used to export the output data to the context. |
Copy link
Member

Choose a reason for hiding this comment

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

Link is missing on runtime expression -> [runtime expression](#runtime-expressions)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that particular link is missed everywhere, maybe should be fixed with a different PR?

@cdavernas
Copy link
Member

Consensus reached! We keep things as @fjtirado commited them! Cheers everyone!

Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
Update dsl-reference.md

Co-authored-by: Ricardo Zanini
<[email protected]>
Update dsl-reference.md

Co-authored-by: Ricardo Zanini
<[email protected]>
[Fix_#873] JpBianchi comments
[Fix_#873] Ricardos comments

Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

Many thanks! ❤️
Sorry for late review though, forgot about this one 👅

@cdavernas cdavernas merged commit 7eb87a8 into serverlessworkflow:main Jun 12, 2024
3 checks passed
cdavernas added a commit to serverlessworkflow/sdk-net that referenced this pull request Jun 19, 2024
fix(Sdk): Updated both InputDataModelDefinition and OutputDataModelDefinition to reflect serverlessworkflow/specification#892
fix(Sdk): Added the ExportDataModelDefinition, and updated TaskDefinition accordingly, to reflect serverlessworkflow/specification#892

Signed-off-by: Charles d'Avernas <[email protected]>
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.

Rename output.from and output.to
5 participants