Skip to content

Canonical SDK feature list - #142

Merged
QuintinWillison merged 140 commits into
mainfrom
sdk-features
Jun 15, 2022
Merged

Canonical SDK feature list#142
QuintinWillison merged 140 commits into
mainfrom
sdk-features

Conversation

@QuintinWillison

@QuintinWillisonQuintinWillison commented Mar 25, 2022

Copy link
Copy Markdown
Contributor

Opus. Early days on this pull request, which I expect to stay in a Draft state for a few days while I work on getting it ready for reviewers. In the meantime, I'm pushing the work up for visibility as I know various people are interested in how this work is evolving.

I expect that I will feel this pull request is ready when the following have been completed:

  • features/README.md populated, including descriptions of:
    • what this is and why it's been created
    • how it's used downstream, or at least how it's anticipated to be used
    • how it intersects with versioning - mentioning client library spec., protocol and SDKs
  • features/ephemeral-notes.md removed
  • Uploads the feature list in human-readable format (likely HTML) to sdk.ably.com, in order to allow us to manual inspect it
  • Illustrates the downstream format anticipated to be used by SDK repositories to indicate which features from this list they implement
  • Worked out whether/how to include the client library spec. and/or protocol version... I feel that the protocol supported by an SDK is a feature

@QuintinWillisonQuintinWillison self-assigned this Mar 25, 2022
…file.
By adding the yaml extension to the prettier tool's remit I had started looping in this file, albeit not on macOS runners (explaining why it was failing in our Linux-based CI runner but not locally for me, macOS-based).
The file .markdownlint-cli2.yaml was being overlooked on macOS because of its dot prefix, despite failing our Linux-based CI.
For what it's worth, I first tried a "more clever" pattern but that caused a different failure:
ably-common % npm run lint
> ably-common@1.2.0 lint
> npm-run-all format:*:check
> ably-common@1.2.0 format:js-code:check
> eslint --max-warnings=0 .
> ably-common@1.2.0 format:data:check
> prettier --check **/{.*,*}.{json,yml,yaml}
Checking formatting...
[error] No files matching the pattern were found: "**/.*.yml".
All matched files use Prettier code style!
ERROR: "format:data:check" exited with 2.
?1
Base automatically changed from eslint to mainMarch 29, 2022 11:37
@github-actions
github-actionsBot temporarily deployed to staging/pull/142/sdk-features March 30, 2022 13:03 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/142/sdk-features March 30, 2022 13:34 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/142/sdk-features March 30, 2022 13:49 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/142/sdk-features March 31, 2022 18:23 Inactive
@github-actions
github-actionsBot temporarily deployed to staging/pull/142/sdk-features April 12, 2022 12:28 Inactive
@QuintinWillison
QuintinWillison removed the request for review from tomczoinkMay 31, 2022 13:53
…ature list.
It's out of scope for the work of establishing canonical feature list foundations.
It's out of scope for the work of establishing canonical feature list foundations.
It's also very likely that this information would live elsewhere, perhaps alongside the SDK interfaces - as annotations on the source code (TBC).
…rpose of the variants property.
This was an oversight on my part while I was prototyping this. I didn't see the wood for the trees.
It's out of scope for the work of establishing canonical feature list foundations.
It's also very likely that this information would live elsewhere, perhaps alongside the SDK interfaces - as annotations on the source code (TBC).
…irements stricter.
Moves the null check from validateStructure function to the more narrowly scoped validateMapItems function.
Adds commentary to explain why value for a key can be null in a map.
Moves from tracking level to tracking parentKeys as we traverse so we can provide useful debug information when the structure is invalid.
The intention with these additional paragraphs is to provide a slightly softer pre-amble to this document, focussed more on the 'why' than the 'what'.
… scope of concern for the features specification.
…ng this repository.
It was confusing and is likely implicit, especially once the features specification moves here.
But, regardless, that's a decision / clarification that does not need to be made today.
… Schemas.
It potentially complicates decisions around versioning.
@QuintinWillison

Copy link
Copy Markdown
ContributorAuthor

@lmars in respect of the conversation we had on yesterday's synchronous call, around versioning this repository, I've added 2c42292. On further reflection, I think it might help if I presented a proposal for the how we might approach this as I still think there could be benefits of 'whole repository version number', even if that could even mean a 'new home' for the schema sources. 🤔 I'll ensure to tag you on that proposal, probably as an issue on this repository, once I create it.

@QuintinWillison

Copy link
Copy Markdown
ContributorAuthor

I've addressed all of the review input I've received that felt like it fell within the scope of this pull request - being the establishment of foundations that can be built on further in subsequent pull requests, either in this repository or elsewhere.

While I'm unavailable for a week or so from tomorrow, @owenpearson has agreed to take on the task of seeing this pull request to the point where it can be landed. I'm going to assign it to him now and will also mark it as 'Ready for Review'. I will leave it up to Owen to re-request review from those who have already provided review input, as well as to request review from those who have not yet contributed but might wish to have the opportunity.

@QuintinWillison

Copy link
Copy Markdown
ContributorAuthor

I'm back from some time off and am now going to re-assign this pull request to myself. @owenpearson and I have discussed the content and have agreed that the ephemeral notes markdown file should be removed, as it's not appropriate for addition to the Git index. If there are any parts of that file which I feel need to outlive this PR then I shall put them in a GitHub issue.

@tomkirbygreentomkirbygreen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass. Will take a more architectural look once I've attended to some other stuff.

Comment threadfeatures/README.md Outdated
Comment threadfeatures/sdk-node-properties.js
@QuintinWillison

Copy link
Copy Markdown
ContributorAuthor

Thank you to everybody who has provided review input and/or approved this work. I'm going to merge it to main branch now, however I am (as ever) always happy to receive posthumous comments as this merely establishes some foundations, so all input no matter how late received is very welcome - there's plenty more work to be done.

@lmarslmars left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a pending review that I forgot to submit, sorry! Only minor though...

- uses: ably/sdk-upload-action@v1
with:
s3AccessKeyId: ${{ secrets.SDK_S3_ACCESS_KEY_ID }}
s3AccessKey: ${{ secrets.SDK_S3_ACCESS_KEY }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should use GitHub OIDC.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks. I've created #154

Sign up for freeto 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.

8 participants

@QuintinWillison@ikbalkaya@lmars@tomkirbygreen@Peter-Maguire@stmoreau@AndyNicks@owenpearson