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

spec: TI5 Clarify when a URL should be appended to log messages #540

Merged
merged 1 commit into from
Nov 15, 2018

Conversation

mattheworiordan
Copy link
Member

Rather belated, but here's my attempt at clarifying when a URL is included in log entries shown to users. Specifically addressing ably/ably-ruby#171 (comment) and ably/ably-ruby@ea425be.

Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

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

Is it worth clarifying what we mean by "If the URL is not already present within the contents of the @message@ attribute ..."

The implementation in Ruby determines what the URL should be, based on the href and/or code, and then looks for that exact URL in the message, appending the URL if that isn't found.

I think this text should make it clear that the "If the URL is not already present .." check requires an exact match, or we should perhaps specify some more relaxed match if we think that's appropriate.
Should we ma

@SimonWoolf
Copy link
Member

given that the 'not already present' requirement is only temporary (we can remove it in the 1.1 spec, since for v=1.1 realtime doesn't append an href to the message, so it's only really applicable for particularly keen client libs that want to implement this spec item before 1.1), probably not worth being too particular about it

@mattheworiordan
Copy link
Member Author

probably not worth being too particular about it

I'd be happy with that 👍

@mattheworiordan mattheworiordan merged commit a91dd3d into master Nov 15, 2018
@mattheworiordan mattheworiordan deleted the further-clarification-on-href-processing branch November 15, 2018 16:00
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