Skip to content

Conversation

frankkilcommins
Copy link
Collaborator

fixes #129

@frankkilcommins frankkilcommins added the implementor-draft In scope for first version label Feb 13, 2024
@handrews
Copy link
Member

I'll get to this tomorrow or at the latest Friday.

@handrews
Copy link
Member

@frankkilcommins Sorry for the delay, I had to go back and reconsider this as I missed some things with my original feedback. For some reason, I had thought this was resolving names from the Components Object (like Security Requirement Objects resolve Security Scheme names from Components).

But now I see that this is a Reference Object, and AFAICT it's the only use of a Reference Object in the spec. So I read through that section, and I'm guessing one reason it's a Reference Object is to allow referencing parameter definitions that go with the target operation, which would require referencing external documents.

Then I thought... since this is the only use of a Reference Object, and everything else is using some form of runtime expression (sometimes with a JSON Pointer), why not get rid of this Reference Object and use a runtime expression instead? With whatever modification you need to do the value override thing.

You would still have JSON Schema references, but they are slightly different anyway.

I apologize for throwing a new idea in at the last minute here, but given the work we've done to avoid using $ref outside of JSON Schema in Moonwalk, since you're already very close to that goal here, why not go the last little bit and do everything with the runtime expression syntax? It's nicer in that it already unambiguously resolves from source documents. (if I am reading all of this correctly - if I'm not making sense please just lmk).

@frankkilcommins
Copy link
Collaborator Author

@handrews I think it's achievable to move in this direction and not have any specification specific Reference Object situations. It means I'll have to re-write some code I build for parsing but such is life 😛

This section will also need an update (it's not correct in it's current state anyway IMO with the exception of a JSON Schema reference but that's not mentioned either).

Instead of the current situation of:

  - $ref: "#/components/parameters/page"
    value: 1

We could have something like (with the appropriate verbiage that the parameter page defined is to be used and the value MUST override that of the evaluated Parameter object):

- $components.parameters.page
  value: 1

@handrews
Copy link
Member

@frankkilcommins that looks great to me! (although you either need to make both of those array entries or give a property name to the $components part, I think?) It will also be really interesting for moonwalk to see how this all-runtime-expression-syntax approach works in practice.

@frankkilcommins
Copy link
Collaborator Author

@handrews finally got around to making this change........no more Reference Object 🚀

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

Generally looks great - my only suggestion is maybe call it a "Parameter Re-Use Object" or something of that sort to avoid it sounding too much like "Reference Object"? But if you prefer the "Parameter Reference Object" name I'd happily approve that.

Copy link
Collaborator

@kevinduffey kevinduffey left a comment

Choose a reason for hiding this comment

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

GTG

@frankkilcommins frankkilcommins merged commit aa47092 into main Mar 28, 2024
frankkilcommins added a commit that referenced this pull request Mar 28, 2024
…170)

* chore: suggest multilevel parameters (#155)

* chore: suggest multilevel parameters

* Update versions/1.0.0.md

Co-authored-by: Frank Kilcommins <[email protected]>

* Update examples/1.0.0/ExtendedParametersExample.workflow.yaml

Co-authored-by: Frank Kilcommins <[email protected]>

* Update versions/1.0.0.md

Co-authored-by: Frank Kilcommins <[email protected]>

---------

Co-authored-by: Frank Kilcommins <[email protected]>

* Remove references to WHATWG to avoid confusion (#145)

* Remove references to WHATWG to avoid confusion

* Correct relative reference wording

* Section 4.2 not 4.1!

* Simplify URI wording

* Adjust Step Parameters desc to cater for Workflow parameters addition (#169)

* feat: add `dependsOn` capability for workflow level (#164)

* feat: add `dependsOn` capability for workflow level

* chore: typo fix

* chore: grammer fix

* feat: Add Request Body Object (#162)

* feat: Add Request Body Object

* chore: fix typos in examples

* Clarity on referencing Components Parameters (#149)

* Clarity on referencing Components Parameters

* Remove Reference Object to avoid clash with JSON Schema keyword. Replace with expression based referencing

* chore: keep fixed field link names consistent

* chore: Name component parameters as type Reusable Parameter Object

* chore: adjust Workflow level parameters to use Reusable Parameter Objects

---------

Co-authored-by: Dmytro Anansky <[email protected]>
Co-authored-by: Nick Denny <[email protected]>
frankkilcommins added a commit that referenced this pull request Apr 3, 2024
* feat: adding support for share success and failure actions

* Merging latest 'main' changes in to Action Object Extensions branch (#170)

* chore: suggest multilevel parameters (#155)

* chore: suggest multilevel parameters

* Update versions/1.0.0.md

Co-authored-by: Frank Kilcommins <[email protected]>

* Update examples/1.0.0/ExtendedParametersExample.workflow.yaml

Co-authored-by: Frank Kilcommins <[email protected]>

* Update versions/1.0.0.md

Co-authored-by: Frank Kilcommins <[email protected]>

---------

Co-authored-by: Frank Kilcommins <[email protected]>

* Remove references to WHATWG to avoid confusion (#145)

* Remove references to WHATWG to avoid confusion

* Correct relative reference wording

* Section 4.2 not 4.1!

* Simplify URI wording

* Adjust Step Parameters desc to cater for Workflow parameters addition (#169)

* feat: add `dependsOn` capability for workflow level (#164)

* feat: add `dependsOn` capability for workflow level

* chore: typo fix

* chore: grammer fix

* feat: Add Request Body Object (#162)

* feat: Add Request Body Object

* chore: fix typos in examples

* Clarity on referencing Components Parameters (#149)

* Clarity on referencing Components Parameters

* Remove Reference Object to avoid clash with JSON Schema keyword. Replace with expression based referencing

* chore: keep fixed field link names consistent

* chore: Name component parameters as type Reusable Parameter Object

* chore: adjust Workflow level parameters to use Reusable Parameter Objects

---------

Co-authored-by: Dmytro Anansky <[email protected]>
Co-authored-by: Nick Denny <[email protected]>

* Add Reusable Object and referencing ability

* chore: fix yaml example indentation

* chore: fix JSON examples

* chore: fix typo in `workflowStepActions`

---------

Co-authored-by: Dmytro Anansky <[email protected]>
Co-authored-by: Nick Denny <[email protected]>
@frankkilcommins frankkilcommins deleted the frankkilcommins/issue129 branch July 31, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementor-draft In scope for first version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarity on referencing Components Parameters
3 participants