-
Notifications
You must be signed in to change notification settings - Fork 687
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
json-functions: Update status and links #8762
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Note that pingcap/tidb#7968 lists this as not-yet-implemented JSON support:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with some suggestion on fixing rendering issues.
@@ -103,6 +98,8 @@ The following JSON functions are unsupported in TiDB. You can track the progress | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this renders correctly, please format this whole paragraph as the two first bullet points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the context of your comment got lost somehow. It also looks like it is already fixed. Could you check if this is really the case and resolve this discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to fix the formatting/rendering of 'See also' paragraph.
@@ -103,6 +98,8 @@ The following JSON functions are unsupported in TiDB. You can track the progress | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this is as intended. The lines like this should not end up in the doc directly: [json_pretty]: https://dev.mysql.com/doc/refman/5.7/en/json-utility-functions.html#function_json-pretty But this allows lines like this to reference the URL: | [JSON_PRETTY(json_doc)][json_pretty] | Pretty formatting of a JSON document | Another option would be to combine the two lines like this: | [JSON_PRETTY(json_doc)][https://dev.mysql.com/doc/refman/5.7/en/json-utility-functions.html#function_json-pretty] | Pretty formatting of a JSON document | But then the markdown table gets difficult to read due to the long lines. The problem with formatting that I fixed was related to this, there were functions that had a reference that was not defined, so it couldn't format the link properly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 2facd3f
|
/cherry-pick release-6.1 v6.1.0
v6.0.0
v5.4.1
v5.3.1
v5.2.4
v5.1.3
v5.0.6
So:
|
@dveeden: new pull request created: #9019. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
@dveeden: new pull request created: #9020. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
@dveeden: new pull request created: #9021. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
@dveeden: new pull request created: #9022. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
@dveeden: new pull request created: #9023. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
I tried a simple script to detect if there are more issues like this in other pages.
However it doesn't look like other pages have the same issue and there are some false positives, so this is not something we can easily add to CI/CD. /\[.*\]\[.*\]/ {
a=substr($0,index($0, "][")+2);
links[substr(a,1,index(a,"]")-1)] = $0
}
/^\[.*\]: / {
targets[substr($0,2,index($0, "]")-2)] = $0
}
END {
for (link in links) {
if (targets[link] == "") {
print "No target defined for \"" link "\"", "Link defined as: " links[link]
}
}
} |
In response to a cherrypick label: new pull request created: #9360. |
What is changed, added or deleted? (Required)
Update the status of JSON functions in TiDB
Which TiDB version(s) do your changes apply to? (Required)
Tips for choosing the affected version(s):
By default, CHOOSE MASTER ONLY so your changes will be applied to the next TiDB major or minor releases. If your PR involves a product feature behavior change or a compatibility change, CHOOSE THE AFFECTED RELEASE BRANCH(ES) AND MASTER.
For details, see tips for choosing the affected versions.
What is the related PR or file link(s)?
Do your changes match any of the following descriptions?