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

Improve the aesthetics of code review #6129

Closed
yangwenmai opened this Issue Mar 23, 2018 · 7 comments

Comments

Projects
None yet
6 participants
@yangwenmai
Copy link
Contributor

yangwenmai commented Mar 23, 2018

Overview of the problem

Some of the % in TiDB's test cases enclose the statement with double quotes "", but Github escaping it when displayed, so it will be marked with % red, and we are doing a review of It doesn't look beautiful at the moment, so we need to change this and recommend replacing it with the Go language.

For examples: PR #5697

How to decide?

I can give you a reference note. If you use the Go syntax highlighting plug-in, then you can see that the unhighlighted part is escaping. It may be problematic. Then we need to make changes. [eg, 100%500, %W %r].

github_tidb_review_3

How to test and verify?

  1. First you have to fork TiDB (so that you can edit the file on GitHub).
  2. Open a suspicious file (For examples: util/stringutil/string_util_test.go).
  3. Find the code statement for the problem overview description.
  4. Modify the 3 corresponding code statement.
  5. Click Preview changes.
  6. Check and verify that your changes are correct.

Let me give you an example. It will be clear to see.

github_tidb_review_1

github_tidb_review_2

to add on

The above content is the original intention that we should change, but in fact we can do more.

Wrap all testkit's pending SQL statements with `` .

Therefore, we should modify the SQL statements of the following methods by the way when we modify them.

  • Exec
  • MustExec
  • MustQuery

List

Reference

https://golang.org/ref/spec#String_literals

@yangwenmai

This comment has been minimized.

Copy link
Contributor Author

yangwenmai commented Mar 23, 2018

I'm working on it (#6130). I will be improve the aesthetics of code review in executor/aggregate_test.go.

@kangxiaoning

This comment has been minimized.

Copy link
Contributor

kangxiaoning commented Mar 24, 2018

I'm working on #6137.
The following file is being processed:

executor/grant_test.go
expression/expr_to_pb_test.go
expression/evaluator_test.go
expression/integration_test.go
expression/typeinfer_test.go
parser/parser_test.go
plan/physical_plan_test.go
session/bootstrap_test.go
session/session_test.go
session/tidb_test.go
types/format_test.go # nothing to do
util/prefix_helper_test.go

We can use the command to find the line number , which need to update.

➜  tidb git:(master) more expression/integration_test.go|grep -n "%"|grep -E '%[^vdHhDhxispMeYmfcaX]'|grep -v -E "'%'|\`"
1381:	result = tk.MustQuery(`SELECT DATE_FORMAT('2017-06-15', '%W %M %e %Y %r %y');`)
1383:	result = tk.MustQuery(`SELECT DATE_FORMAT(151113102019.12, '%W %M %e %Y %r %y');`)
1385:	result = tk.MustQuery(`SELECT DATE_FORMAT('0000-00-00', '%W %M %e %Y %r %y');`)
2127:		{`aA%`, "aAab", 1},
➜  tidb git:(master)
@chenyang8094

This comment has been minimized.

Copy link
Contributor

chenyang8094 commented Mar 24, 2018

I'm working on #6138. I will be improve the aesthetics of code review in executor/show_test.go 、 expression/bench_test.go 、expression/builtin_string_test.go and expression/builtin_time_test.go.

@hechen0

This comment has been minimized.

Copy link
Contributor

hechen0 commented Mar 25, 2018

#6144. improved util/ranger/ranger_test.go

@yangwenmai

This comment has been minimized.

Copy link
Contributor Author

yangwenmai commented Mar 27, 2018

@tiancaiamao The issue todo list has not been resolved and should not be closed.

@tiancaiamao tiancaiamao reopened this Mar 27, 2018

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

tiancaiamao commented Mar 27, 2018

@yangwenmai
Actually, it's closed by github robot.
If you send a PR and write description like fix #6129, when the PR is merged, the associating issue would be closed automatically.

@yangwenmai

This comment has been minimized.

Copy link
Contributor Author

yangwenmai commented Mar 27, 2018

Got it.

@ngaut ngaut closed this Apr 1, 2018

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