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

[doc] Revise the CLI documentation #5840

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

tengqm
Copy link
Contributor

@tengqm tengqm commented Dec 11, 2024

This PR updates the CLI documentation.
The revised version retains all existing information, and the major
revision is about simplification and fixing ambiguity or typo errors.

@tengqm
Copy link
Contributor Author

tengqm commented Dec 11, 2024

To reviewers: checking the git diff is not an efficient way to review for this PR (drastical change). Maybe a direct checking of the rendered output can help save some energy.

This document provides guidance on managing metadata within Apache Gravitino using the Command Line Interface (CLI). The CLI offers a terminal based alternative to using code or the REST interface for metadata management.
This document provides a guidance on managing metadata within Apache Gravitino using
the Command Line Interface (CLI). The CLI offers a terminal based alternative to
using code or the REST API for metadata management.
Copy link
Member

Choose a reason for hiding this comment

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

Please don't break up sentences, this stops some IDE tools like grammar checkers from working and makes it easier for errors to slip into the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Em... Let me try articulate the reasons behind this kind of changes.

  • We'd better treat docs as code. That means we write them and then track whatever
    changes in the future.
  • The GIT tool compare changes on a line by line basis. When we have a long line of
    text, GIT can only tell you that this "line" has changed. We as human reviewers have
    to identify the actual change(s) manually. That means a painful experience like finding
    a needle fallen on the ground.
  • The GitHub web UI works the same way. I have myself been tortured by this for many years.
  • When we break long lines manually, appropriately, this line breaks are not impacting
    the rendered output (mainly HTML here).
  • With manual line breaks, we ensure that every developer sees the same thing on his
    or her environment. Think about the tab size problem here.
  • I am not quite sure how an IDE helps improve the documentation. Even if there is an
    IDE (say IDEA) can do a great job for something, we are not writing code/docs for
    that IDE. Human is not supposed to be hijacked by tools, right?
  • I'm open to learn the draw backs of manually wrapped long lines. In some other
    communities, manually wrapping long lines is strongly encouraged.
  • This does NOT mean we want to encourage people to break the long lines at a fix boundary.
    The recommended way is to break them WHEN APPROPRIATE. For example, we encourage
    breaking long lines when a statement ends, when there is a punctuation character,
    so on and so forth. We discourage breaking links, however, because it doesn't make
    a lot of senses.

Copy link
Member

@justinmclean justinmclean Dec 11, 2024

Choose a reason for hiding this comment

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

The GitHub web UI doesn't work the same way it highlights word changes within a line.

We are writing docs in an IDE, and IDE plugins such as grammar checkers help do that. They don't always work if you break sentences over lines.

In this project, I noticed that in the documentation that breaks up sentences, errors are easily introduced in changes (as they have been here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Let's use the change I made as an example. GitHub failed to identify that this change is only about inserting several line breaks.

Copy link
Member

Choose a reason for hiding this comment

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

That shows no changes other than the newlines or lines moved. This is a better example:
image

Copy link
Member

@justinmclean justinmclean Dec 12, 2024

Choose a reason for hiding this comment

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

Here, you can see two words have been changed, and I know no other changes have been made.

docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
- a string format boolean indicating whether the column can be null (default=true)
- a string format boolean indicating whether the column is auto-incremented (default=false)
- the default value
- the default data type, defaults to the column data type if a default value is specified.
Copy link
Member

@justinmclean justinmclean Dec 11, 2024

Choose a reason for hiding this comment

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

no need for a comma and "if a default value is ONLY specified"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we'd better mention that, because we are not saying
"if a default type is specified".
The defaulting of "type" depends on the setting of "value".
We need to highlight this.

Copy link
Member

Choose a reason for hiding this comment

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

You need to specify the column data type and optionally a default value. If you specify the value you can optionally set the data type of that value. If you do not specify the value's default type, it will use the column's data type. They are likely to be the same in 99% of cases, I'd guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try rephrase this ... you mean the following is true no matter a default value is specified or not, right?

the default data type, defaults to the column data type

Copy link
Member

Choose a reason for hiding this comment

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

No as if you don't specify a default value, there is no default value to have a default type. If you do specify a default value but not a default type it will default to the column's data type, OR you can specify a default value and a default type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you specify a default value, then the default column type defaults to the column data type

Yes. That is exactly what I captured from your clarification.
Maybe you can provide a clearer description?

BTW, this logic is ugly and needs some rethinking. Are we sure we want to do this?
You can see I have had a difficult time to understand this logic.
The same difficulty will be a pain for other users/developers.
If we have a logic that is so difficult to explain, I'd suggest we drop this smart hack.

Copy link
Member

Choose a reason for hiding this comment

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

I'll come up with some better text. In 99% of cases, the column will be omitted, I think most people will just follow the example and not even notice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent some time rethinking the issue...
Maybe we should not do this defaulting. If a user is not specifying default data type,
then it is not specified. Period.
This extra logic of inference is not 100% reliable. It adds extra complexity,
which is difficult to explain. Why are we doing this then...?

Copy link
Member

Choose a reason for hiding this comment

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

We could just say "the default data type (optional)" as that would cover 95% of cases and cause no confusion

Copy link
Member

Choose a reason for hiding this comment

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

If a user doesn't specify the default type then it is not defined as there is no default type so we are currently doing exactly what you are saying.

docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
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.

2 participants