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

charset: clean up some code about charset #1306

Merged
merged 5 commits into from
Aug 16, 2021

Conversation

xiongjiwei
Copy link
Contributor

@xiongjiwei xiongjiwei commented Aug 13, 2021

What problem does this PR solve?

  • use a map to store the charset info directly
  • remove Desc struct, it is definitely same with Charset

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 13, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • tangenta
  • xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@xiongjiwei xiongjiwei marked this pull request as ready for review August 13, 2021 08:01
@xiongjiwei xiongjiwei requested review from bb7133 and hawkingrei and removed request for bb7133 August 13, 2021 08:01
@xiongjiwei xiongjiwei changed the title charset: add a switch to control the charset feature charset: clean up some code about charset Aug 13, 2021
charset/charset_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

There are a few exported functions/struct changes:

  • Desc/GetCharsetDesc
  • GetSupportedCharsets
  • GetCharsetInfo

This requires the users to change the code when they upgrade the dependency. I think this is acceptable since the changes should be easy. Please also update the TiDB part.

Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Do we also cleanup collation same way?

@ti-chi-bot ti-chi-bot added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Aug 16, 2021
@xiongjiwei
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: caee1c0

@ti-chi-bot ti-chi-bot merged commit a9d82a5 into pingcap:master Aug 16, 2021
@xiongjiwei
Copy link
Contributor Author

Do we also cleanup collation same way?

I think the whole file should be rewritten. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants