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.

Sign up for GitHub

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

Jump to bottom

Add ably agent header #188

Merged
merged 10 commits into from Oct 27, 2021
Merged

Add ably agent header #188

merged 10 commits into from Oct 27, 2021

Conversation

ben-xD
Copy link
Contributor

@ben-xD ben-xD commented Oct 20, 2021

No description provided.

@ben-xD ben-xD changed the base branch from main to feature/update-ably-cocoa-1.2.6 October 20, 2021 13:31
@github-actions github-actions bot temporarily deployed to staging/pull/188/dartdoc October 20, 2021 13:31 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/188/dartdoc October 20, 2021 14:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/188/dartdoc October 20, 2021 14:18 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/188/dartdoc October 20, 2021 14:32 Inactive
QuintinWillison
QuintinWillison suggested changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

I would perhaps consider renaming LIBRARY_VERSION to PLUGIN_VERSION as that is, perhaps, clearer (given the fact that this codebase is very reliant on dependencies that can definitely be referred to a 'library' also... context is everything 🤔).

android/ably-agent.gradle Outdated Show resolved Hide resolved
example/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to staging/pull/188/dartdoc October 20, 2021 14:41 Inactive
@ben-xD
Copy link
Contributor Author

ben-xD commented Oct 20, 2021 •
edited

I would perhaps consider renaming LIBRARY_VERSION to PLUGIN_VERSION as that is, perhaps, clearer (given the fact that this codebase is very reliant on dependencies that can definitely be referred to a 'library' also... context is everything 🤔).

I'm not sure. If we call it a plugin, then someone could ask the question, whats the difference between the plugin and ably_flutter. We could also call it a package version, as that is more dart-like. Or even, Dart Package version.

Perhaps BuildConfig.DART_PACKAGE_VERSION would be good. Renamed to FLUTTER_PACKAGE_PLUGIN_VERSION

@github-actions github-actions bot temporarily deployed to staging/pull/188/dartdoc October 20, 2021 14:46 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/188/dartdoc October 20, 2021 14:57 Inactive
owenpearson
owenpearson approved these changes Oct 20, 2021
View reviewed changes
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

The stuff I understand looks good 🙂

example/ios/Podfile.lock Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to staging/pull/188/dartdoc October 20, 2021 18:01 Inactive
@jamienewcomb
Copy link
Member

Can we make sure all PRs link to an issue please
this one should be linked to #100 - there is some good guidance on this here https://ably.atlassian.net/wiki/spaces/PUB/pages/801046901/Git+PR+Best+Practice

Base automatically changed from feature/update-ably-cocoa-1.2.6 to main October 27, 2021 17:43
@QuintinWillison QuintinWillison linked an issue Oct 27, 2021 that may be closed by this pull request
Implement RSC7d (Ably-Agent header) #100
Closed
@QuintinWillison QuintinWillison merged commit 58930ef into main Oct 27, 2021
@QuintinWillison QuintinWillison deleted the feature/ably-agent-header branch October 27, 2021 17:46
@QuintinWillison QuintinWillison mentioned this pull request Jan 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuintinWillison QuintinWillison QuintinWillison approved these changes

@owenpearson owenpearson owenpearson approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement RSC7d (Ably-Agent header)
4 participants
@ben-xD @jamienewcomb @QuintinWillison @owenpearson