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

connect: expanded formatting modes #535

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

GRISHNOV
Copy link
Contributor

@GRISHNOV GRISHNOV commented Jul 16, 2023

The patch introduces formatting modes for the interactive console [1].

Closes #162

@TarantoolBot document
Title: Expanded formatting modes

The patchset introduces addition commands for the tt connect console.

It adds several common commands that relate to a general functionality:

  • \help, ? - to show help message with additional information and a list of all commands.
  • \quit, \q - quit from the console. Previously this command worked on the tarantool side, but now it works on the tt side and does not require execution rights for a user.

In addition, the patch adds commands for extended output modes:

  • \set output <format> - set format lua, table, ttable or yaml (default)
  • \set table_format <format> - set table format default, jira or markdown
  • \set graphics <false/true> - disables/enables (default) pseudographics for table modes
  • \set table_column_width <width> - set max column width for table/ttable
  • \xw <width> - set max column width for table/ttable
  • \x - switches output format cyclically
  • \x[l,t,T,y] - set output format lua, table, ttable or yaml
  • \x[g,G] - disables/enables pseudographics for table modes

Supported formats:

  1. yaml - shows data as a YAML document. The format is used now by tarantool and tt.
  2. lua - shows data as Lua-compatible structs.
  3. table - shows data as a pseudo graphical table.
  4. ttable - the same as the table format, but a result table is transposed (rotate data from rows to columns).

Supported table formats (for table or ttable formats):

  1. default - shows data as a pseudo graphical table.
  2. markdown - shows data as a Markdown-compatible table.
  3. jira - shows data as a Jira-compatible table.

Example:

> \set output table
> box.space.customers:select()
+------+-----------+------+
| col1 | col2      | col3 |
+------+-----------+------+
| 1    | Elizabeth | 12   |
+------+-----------+------+
> \xg
> box.space.customers:select()
 col1  col2       col3 
 1     Elizabeth  12   
 2     Mary       46   
 3     David      33

@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-162-expanded-formatting-modes branch from e1e2335 to e38eca8 Compare July 17, 2023 11:20
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

After discussion with @LeonidVas and @psergee we decided that an import of the go-pretty code should be done in a different way:

  1. Create a patch for the latest release of the go-pretty. The patch should be as small as possible.
  2. Add go-pretty as a git submodule.
  3. Patch the submodule with the patch on the build step.

See how it is already done with cartridge-cli in the repo.

cli/connect/interactive.go Outdated Show resolved Hide resolved
cli/connect/interactive.go Outdated Show resolved Hide resolved
cli/connect/interactive.go Outdated Show resolved Hide resolved
cli/connect/console.go Outdated Show resolved Hide resolved
cli/connect/interactive.go Outdated Show resolved Hide resolved
cli/formatter/formatter.go Outdated Show resolved Hide resolved
cli/formatter/formatter.go Outdated Show resolved Hide resolved
cli/formatter/formatter.go Outdated Show resolved Hide resolved
cli/formatter/formatter.go Outdated Show resolved Hide resolved
cli/formatter/table/README.md Outdated Show resolved Hide resolved
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-162-expanded-formatting-modes branch 4 times, most recently from 68a7c8e to 2a0519c Compare July 20, 2023 20:30
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Thank you for the update, I have a couple of questions about the go-pretty patching.

cli/formatter/format.go Outdated Show resolved Hide resolved
@oleg-jukovec oleg-jukovec self-requested a review July 21, 2023 18:47
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Approved by mistake.

@oleg-jukovec oleg-jukovec self-requested a review July 21, 2023 18:48
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-162-expanded-formatting-modes branch 3 times, most recently from 2f62466 to cef8fdc Compare July 23, 2023 10:23
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

I'm glad to see that we could resume without the go-pretty patching. Let's now focus on code style.

cli/connect/console.go Outdated Show resolved Hide resolved
cli/connect/interactive.go Outdated Show resolved Hide resolved
cli/connect/interactive.go Outdated Show resolved Hide resolved
cli/formatter/format.go Outdated Show resolved Hide resolved
cli/formatter/format.go Outdated Show resolved Hide resolved
cli/formatter/types.go Outdated Show resolved Hide resolved
cli/formatter/types.go Outdated Show resolved Hide resolved
cli/formatter/types.go Outdated Show resolved Hide resolved
cli/formatter/types.go Outdated Show resolved Hide resolved
cli/formatter/types.go Outdated Show resolved Hide resolved
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-162-expanded-formatting-modes branch from cef8fdc to 104246c Compare July 24, 2023 12:07
cli/formatter/format_test.go Outdated Show resolved Hide resolved
cli/formatter/format.go Outdated Show resolved Hide resolved
cli/connect/interactive.go Outdated Show resolved Hide resolved
cli/formatter/format.go Outdated Show resolved Hide resolved
cli/formatter/format.go Outdated Show resolved Hide resolved
cli/formatter/format.go Outdated Show resolved Hide resolved
cli/formatter/format.go Outdated Show resolved Hide resolved
@oleg-jukovec oleg-jukovec self-requested a review July 24, 2023 14:34
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Some more questions.

@GRISHNOV
Copy link
Contributor Author

#535 (comment)

But it covered by integration tests, why duplicate?

@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-162-expanded-formatting-modes branch from 104246c to a4647e8 Compare July 26, 2023 16:34
@oleg-jukovec oleg-jukovec self-requested a review July 26, 2023 16:54
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-162-expanded-formatting-modes branch 2 times, most recently from f79de6e to 52e35ce Compare July 30, 2023 08:01
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

The critical lack of the implementation is the input data for the MakeOutput function inconsistent with all formats.

{ // Ok.
	formatter.DefaultFormat,
	"---\n- 1\n- 2\n- 3\n...",
	"---\n- 1\n- 2\n- 3\n...\n",
	false,
},
{ // Ok.
	formatter.TableFormat,
	"---\n- 1\n- 2\n- 3\n...",
	"+------+\n" +
		"| col1 |\n" +
		"+------+\n" +
		"| 1    |\n" +
		"+------+\n" +
		"| 2    |\n" +
		"+------+\n" +
		"| 3    |\n" +
		"+------+\n",
	false,
},
{ // The problem. It does not print a tuple.
	formatter.DefaultFormat,
	"---\n- DATA: 8\n  COLUMN_1: 30\n  FOO: 1\n  COLUMN_2: 50\n...",
	"---\n- DATA: 8\n  COLUMN_1: 30\n  FOO: 1\n  COLUMN_2: 50\n...\n",
	false,
},
{ // Ok.
	formatter.TableFormat,
	"---\n- DATA: 8\n  COLUMN_1: 30\n  FOO: 1\n  COLUMN_2: 50\n...\n",
	"+----------+----------+------+-----+\n" +
		"| COLUMN_1 | COLUMN_2 | DATA | FOO |\n" +
		"+----------+----------+------+-----+\n" +
		"| 30       | 50       | 8    | 1   |\n" +
		"+----------+----------+------+-----+\n",
	false,
},

As a user, I expect to pass a valid data to it, I will get a valid result as tuples for all formats. But in practice, not all formats can work with the same input data. As example, DefaultFormat/YamlFormat could not work with a data with fields names as expected (return tuples without field names in the format).

This will overсomplicate development in the future. We will need to build two different YAML documents for export tuples from IPROTO.

So we need to make sure that the same valid input is valid for all formats, and formatted correctly by them.

@oleg-jukovec oleg-jukovec force-pushed the igrishnov/gh-162-expanded-formatting-modes branch 10 times, most recently from 08782df to 451fe0c Compare September 14, 2023 08:23
@oleg-jukovec oleg-jukovec requested review from oleg-jukovec and removed request for oleg-jukovec September 14, 2023 08:35
@oleg-jukovec oleg-jukovec force-pushed the igrishnov/gh-162-expanded-formatting-modes branch from 451fe0c to f3d4709 Compare September 14, 2023 08:41
@oleg-jukovec
Copy link
Contributor

oleg-jukovec commented Sep 14, 2023

I did a little code refactoring (but not tests refactoring). I suggest to forget about everything that happened before and start the review all over again.

Copy link
Contributor

@DerekBum DerekBum left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I've got some questions, please see the comments below.

Also, are changes from x = x + ... to x += ... and x++ considered as too sinister nits? I just think that it would be nice to see a uniform code style (and the method of addition differs from line to line).

cli/connect/commands.go Outdated Show resolved Hide resolved
cli/connect/commands.go Outdated Show resolved Hide resolved
test/integration/connect/test_connect.py Show resolved Hide resolved
cli/connect/commands.go Outdated Show resolved Hide resolved
cli/connect/commands.go Outdated Show resolved Hide resolved
cli/formatter/lua.go Outdated Show resolved Hide resolved
cli/formatter/lua.go Outdated Show resolved Hide resolved
cli/formatter/lua.go Outdated Show resolved Hide resolved
cli/formatter/lua.go Outdated Show resolved Hide resolved
cli/formatter/table.go Outdated Show resolved Hide resolved
@oleg-jukovec oleg-jukovec force-pushed the igrishnov/gh-162-expanded-formatting-modes branch from f3d4709 to c2295db Compare September 20, 2023 08:12
The patch adds a `\help` command to the `tt connect` commands. It also
improves an internal logic of the command processing.

Part of #162
The command already exists in the Tarantool console, but it requires
execute right to be executed. We add the same command to `tt` and
now everebody could leave `tt connect` console without errors.

Part of #162
@oleg-jukovec oleg-jukovec force-pushed the igrishnov/gh-162-expanded-formatting-modes branch from c2295db to c140f0b Compare September 20, 2023 08:17
Copy link
Contributor

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

LGTM =)

@oleg-jukovec oleg-jukovec force-pushed the igrishnov/gh-162-expanded-formatting-modes branch from c140f0b to d9731de Compare September 21, 2023 03:50
The patch introduces formatting modes for the interactive console [1].

1. https://www.notion.so/tarantool/TT-connect-table-output-format-like-fselect-70bd44d1f77b40cf87eddb357adaf37a

Closes #162

@TarantoolBot document
Title: Expanded formatting modes

The patchset introduces addition commands for the `tt connect` console.

It adds several common commands that relate to a general functionality:

* `\help`, `?` - to show help message with additional information and
  a list of all commands.
* `\quit`, `\q` - quit from the console. Previously this command worked
  on the `tarantool` side, but now it works on the `tt` side and does
  not require execution rights for a user.

In addition, the patch adds commands for extended output modes:

* `\set output <format>` - set format lua, table, ttable or
  yaml (default)
* `\set table_format <format>` - set table format default, jira
  or markdown
* `\set graphics <false/true>` - disables/enables (default)
  pseudographics for table modes
* `\set table_column_width <width>` - set max column width for
  table/ttable
* `\xw <width>` - set max column width for table/ttable
* `\x` - switches output format cyclically
* `\x[l,t,T,y]` - set output format lua, table, ttable or yaml
* `\x[g,G]` - disables/enables pseudographics for table modes

Supported formats:

1. `yaml` - shows data as a YAML document. The format is
   used now by `tarantool` and `tt`.
2. `lua` - shows data as Lua-compatible structs.
3. `table` - shows data as a pseudo graphical table.
4. `ttable` - the same as the table format, but a result table is
   transposed (rotate data from rows to columns).

Supported table formats (for `table` or `ttable` formats):

1. `default` - shows data as a pseudo graphical table.
2. `markdown` - shows data as a Markdown-compatible table.
3. `jira` - shows data as a Jira-compatible table.

Example:

```
> \set output table
> box.space.customers:select()
+------+-----------+------+
| col1 | col2      | col3 |
+------+-----------+------+
| 1    | Elizabeth | 12   |
+------+-----------+------+
> \xg
> box.space.customers:select()
 col1  col2       col3
 1     Elizabeth  12
 2     Mary       46
 3     David      33
```
@LeonidVas LeonidVas merged commit 5189d4f into master Sep 25, 2023
14 checks passed
@LeonidVas LeonidVas deleted the igrishnov/gh-162-expanded-formatting-modes branch September 25, 2023 13:44
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.

Expanded formatting modes
5 participants