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

Switch the default mode of SUBSTRING to be the 'legacy' mode #1735

Merged

Conversation

big-andy-coates
Copy link
Contributor

@big-andy-coates big-andy-coates commented Aug 14, 2018

NEW Description

Rather than switching the default mode of SUBSTRING to be legacy, as this PR originally intended, it now just leaves it as it was, (defaulting to the new mode), as per discussion here: #1682 (comment)

This PR now just contains some tidy up and test changes.

OLD Description

As per discussion here: #1682 (comment)

Switch the default mode of SUBSTRING to be the 'legacy' mode initially, so as not to be a breaking change. We will flip the default, to favour the new impl, in a future release.

Original SUBSTRING change is in #1635.

Please also review the doc changes here: https://github.com/confluentinc/docs/pull/1095

Testing done

Unit / functional.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

…y, so as not to be a breaking change.

Original code change is in confluentinc#1635.
@big-andy-coates big-andy-coates requested review from JimGalasyn and a team August 14, 2018 14:27
| | | by setting |
| | | **Deprecated**: As of version 5.1 of KSQL the |
| | | above version of ``SUBSTRING``is deprecated. |
| | | Please set |
Copy link
Member

Choose a reason for hiding this comment

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

Usually, we don't want to use the word "please" in docs. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorted

Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

LGTM, with one suggestion.

Copy link
Contributor

@hjafarpour hjafarpour left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

LGTM

@big-andy-coates big-andy-coates merged commit 5def8c4 into confluentinc:master Aug 16, 2018
@big-andy-coates big-andy-coates deleted the default_substring_to_legacy branch August 16, 2018 12:52
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.

4 participants