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

Factor out all the common parameters of the various /relations apis #1745

Merged
merged 3 commits into from Mar 19, 2024

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Mar 12, 2024

This doesn't currently work, perhaps because the template needs to resolve refs?

Things this PR needs:

Preview: https://pr1745--matrix-spec-previews.netlify.app

@dbkr
Copy link
Member Author

dbkr commented Mar 12, 2024

Apparently: "the ref partial doesn't support references using fragments yet"

@dbkr dbkr closed this Mar 12, 2024
dbkr added a commit that referenced this pull request Mar 12, 2024
This writes up matrix-org/matrix-spec-proposals#3981

Hopefully this is relatively straightforward, apart from having to add
the parameters and response field in all three places. I tried to factor
these out but it seems references just aren't supported in the right
places currently (see #1745
for my efforts). Path parameters can't be optional, so it can't be done
that way either.
@dbkr dbkr mentioned this pull request Mar 12, 2024
@richvdh richvdh reopened this Mar 13, 2024
@richvdh
Copy link
Member

richvdh commented Mar 13, 2024

Hopefully I've fixed this in #1749, which I have now merged in here.

@dbkr
Copy link
Member Author

dbkr commented Mar 18, 2024

Awesome, thank you for doing that. So this just needs a newsfile and it's ready to go?

@dbkr
Copy link
Member Author

dbkr commented Mar 18, 2024

Ah no, I'll add a checklist to the description.

@richvdh
Copy link
Member

richvdh commented Mar 18, 2024

sorry, I'm still thrashing this out with @zecakeh

@zecakeh
Copy link
Contributor

zecakeh commented Mar 18, 2024

For info, I did a bit more refactoring in zecakeh@bd54781 to check more uses of $ref.

@richvdh
Copy link
Member

richvdh commented Mar 19, 2024

Right, #1751 has landed, so I'm going to rebase this again

This doesn't currently work, perhaps because the template needs to
resolve refs?

Also, the other instances of the params need to be replaced with the
refs and the response fields probably should be factored out too.
@richvdh richvdh force-pushed the dbkr/relations_common_params branch from 089163a to 5a442c4 Compare March 19, 2024 15:02
@richvdh richvdh marked this pull request as ready for review March 19, 2024 15:11
@richvdh richvdh requested a review from a team as a code owner March 19, 2024 15:11
@richvdh richvdh merged commit bb4003a into main Mar 19, 2024
12 checks passed
@richvdh richvdh deleted the dbkr/relations_common_params branch March 19, 2024 17:03
dbkr added a commit that referenced this pull request Mar 19, 2024
* Spec for MSC3981

This writes up matrix-org/matrix-spec-proposals#3981

Hopefully this is relatively straightforward, apart from having to add
the parameters and response field in all three places. I tried to factor
these out but it seems references just aren't supported in the right
places currently (see #1745
for my efforts). Path parameters can't be optional, so it can't be done
that way either.

* Missed schemas

* newsfile

* Actually it clearly isn't going to support markdown, is it?

* grammar

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* grammar

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* Clarity

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* Clarity

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* Typo

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

* More clarity.

Note this is counter what the MSC actually proposed to add, but
I think it's clear that this is what it meant.

---------

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
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.

None yet

3 participants