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

Merge Integration/3.0.0 into main #153

Merged
merged 14 commits into from
Dec 5, 2023
Merged

Conversation

owenpearson
Copy link
Member

No description provided.

lawrence-forooghian and others added 8 commits May 1, 2023 14:20
This partially reverts commit 63affff.

We removed this property in cf35f3d but subsequently re-introduced it in
63affff after deciding that we did not want to make breaking API changes in
specification version 2. Now that we’re starting work on version 3, we can
remove it again.

In line with the guidance of CONTRIBUTING.md, which says that a specification
change that necessitates a major specification version increase should be
accompanied by said increase, here we bump the major version to 3.

Note that since we’re currently working on an integration branch for
specification version 3 and development is still ongoing on version 2, the
specification version number mentioned in RTN16m* may need to be updated before
merging the integration branch into `main`.
This reverts commit 285e375 (but using the latest guidance for how to document
a replaced spec point).

We introduced this functionality in 1197e58 but subsequently removed it in
285e375 after deciding that we did not want to make breaking API changes in
specification version 2. Now that we’re starting work on version 3, we can
reintroduce it.

Same comment as 69e1973 re needing to update "replaced in" spec version before
merging integration branch into `main` applies here too.
CSV2a says that the protocol version is an integer.
Reintroduce breaking API changes for specification 3.0.0
There’s still a question of how this method is meant to behave if the user also
specifies an `X-Ably-Header` header in the `headers` parameter. But it’s a
question that equally applied before adding the `version` parameter, so I’ve
captured it in a separate issue #144.

Same comment as 69e1973 re needing to update "replaced in" spec version before
merging integration branch into `main` applies here too.
Tweak spec for `Rest.request`’s `version` param
Copy link
Member

@mattheworiordan mattheworiordan left a comment

Choose a reason for hiding this comment

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

Nice to see this work making it through 👍

Asked a few small questions, thanks.

textile/features.textile Show resolved Hide resolved
textile/features.textile Show resolved Hide resolved
I think this should have been done in a731d12; Stats.entries is just a Dict and
so presumably there’s no special handling of absent values.
@lawrence-forooghian lawrence-forooghian marked this pull request as draft December 1, 2023 18:56
@lawrence-forooghian
Copy link
Collaborator

Converting this one to a draft whilst sorting out protocol version stuff.

…-into-3.0.0

I’ve accepted the spec version of 2.1.0, which clashes with the version of
3.0.0 that I introduced in 69e1973. I believe that my version bump in that
commit was a mistake due to a misunderstanding of the CONTRIBUTING guidelines.
@lawrence-forooghian
Copy link
Collaborator

I'm going to merge this PR — the only reason the integration/3.0.0 branch existed was so that we could first of all do work for 2.1.0 on main. Now that 2.1.0 has been released, we can continue the 3.0.0 work on main, and do any final review of 3.0.0 upon creating the 3.0.0 release PR.

@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review December 5, 2023 13:27
@lawrence-forooghian lawrence-forooghian merged commit aec47a3 into main Dec 5, 2023
2 checks passed
@lawrence-forooghian lawrence-forooghian deleted the integration/3.0.0 branch December 5, 2023 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants