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

usage formatting #360

Merged
merged 8 commits into from
May 30, 2023
Merged

usage formatting #360

merged 8 commits into from
May 30, 2023

Conversation

JyotikaGargg
Copy link
Contributor

No description provided.

@JyotikaGargg JyotikaGargg added this to the May 2023 Release milestone May 25, 2023
@stuartpa
Copy link
Collaborator

There is a bug for flags that do not have a short cut, they print out as just a "-":

-, --version
Print version information and exit

-, --driver-logging-level
Level of mssql driver messages to print.

@stuartpa
Copy link
Collaborator

Kubectl is doing a paragraph align of the descriptions, e.g.

Options:
    -c, --container='':
        Container name. If omitted, use the kubectl.kubernetes.io/default-container annotation for
        selecting the container to be attached or the first container in the pod will be chosen

    --no-preserve=false:
        The copied file/directory's ownership and permissions will not be preserved in the
        container

Where as this PR is wrapping to col 1, e.g.

  -G, --use-aad
      Tells sqlcmd to use Active Directory authentication. If no user name is provided, authentication method ActiveDirectoryDefault is used. If a password is provided, ActiveDirectoryPassword is used. Otherwise ActiveDirectoryInteractive is used.

  -E, --use-trusted-connection
      Uses a trusted connection instead of using a user name and password to sign in to SQL Server, ignoring any environment variables that define user name and password.

  -U, --user-name
      The login name or contained database user name.  For contained database users, you must provide the database name option

Can we align the description text as well please.

@stuartpa
Copy link
Collaborator

Given the sheer number of flags, it would be nice to be able to put the flags into groups. It looks like Cobra doesn't support this yet, this PR would need to be merged:

spf13/cobra#1778

Is there any other easy way to provide flag grouping that you know of?

@JyotikaGargg
Copy link
Contributor Author

Updated the usage formatting similar to kubectl:
-d, --database-name
--This option sets the sqlcmd scripting variable
SQLCMDDBNAME. This parameter specifies the initial
database. The default is your login's
default-database property. If the database does not
exist, an error message is generated and sqlcmd
exits.

-X, --disable-cmd-and-warn
--Disables commands that might compromise system security.
Sqlcmd issues a warning and continues.

@JyotikaGargg
Copy link
Contributor Author

There is a bug for flags that do not have a short cut, they print out as just a "-":

-, --version Print version information and exit

-, --driver-logging-level Level of mssql driver messages to print.

Resolved this.

@stuartpa
Copy link
Collaborator

There is a bug for flags that do not have a short cut, they print out as just a "-":
-, --version Print version information and exit
-, --driver-logging-level Level of mssql driver messages to print.

Resolved this.

Looks much better:

-c, --batch-terminator
--Specifies the batch terminator. The default value is GO

-s, --column-separator
--Specifies the column separator character. Sets the
SQLCMDCOLSEP variable.

There is an extra "--" at the beginning of each description...

@stuartpa
Copy link
Collaborator

Also, while you here, can you fix:

  --Specifies the SQL authentication method to use to connect
    to Azure SQL Database. One
of:ActiveDirectoryDefault,ActiveDirectoryIntegrated,ActiveDirectoryPassword,ActiveDirectoryInteractive,ActiveDirectoryManagedIdentity,ActiveDirectoryServicePrincipal,SqlPassword

So there are some spaces in the string to it can wrap correctly.

@JyotikaGargg
Copy link
Contributor Author

Also, while you here, can you fix:

  --Specifies the SQL authentication method to use to connect
    to Azure SQL Database. One
of:ActiveDirectoryDefault,ActiveDirectoryIntegrated,ActiveDirectoryPassword,ActiveDirectoryInteractive,ActiveDirectoryManagedIdentity,ActiveDirectoryServicePrincipal,SqlPassword

So there are some spaces in the string to it can wrap correctly.

Resolved

@JyotikaGargg
Copy link
Contributor Author

There is a bug for flags that do not have a short cut, they print out as just a "-":
-, --version Print version information and exit
-, --driver-logging-level Level of mssql driver messages to print.

Resolved this.

Looks much better:

-c, --batch-terminator --Specifies the batch terminator. The default value is GO

-s, --column-separator --Specifies the column separator character. Sets the SQLCMDCOLSEP variable.

There is an extra "--" at the beginning of each description...

Resolved

@stuartpa
Copy link
Collaborator

This is looking much much better. Nit: can you bring the help description back to spaces, so it is the same as kubectl, e.g.:

'''
Options:
-c, --container='':
Container name. If omitted, use the kubectl.kubernetes.io/default-container annotation for
selecting the container to be attached or the first container in the pod will be chosen

--no-preserve=false:
    The copied file/directory's ownership and permissions will not be preserved in the
    container

@JyotikaGargg
Copy link
Contributor Author

@stuartpa
Are you talking about the description should look like below :

Flags:
-K,--application-intent
Declares the application workload type when connecting to a
server. The only currently supported value is ReadOnly. If
-K is not specified, the sqlcmd utility will not support
connectivity to a secondary replica in an Always On
availability group

      --authentication-method
Specifies the SQL authentication method to use to connect
to Azure SQL Database. One of: ActiveDirectoryDefault,
ActiveDirectoryIntegrated, ActiveDirectoryPassword,
ActiveDirectoryInteractive, ActiveDirectoryManagedIdentity,
ActiveDirectoryServicePrincipal, SqlPassword

-c,--batch-terminator
Specifies the batch terminator. The default value is GO

beacuse right now it looks like this :

Flags:
  -K, --application-intent
        Declares the application workload type when connecting to a
        server. The only currently supported value is
        ReadOnly. If -K is not specified, the sqlcmd
        utility will not support connectivity to a
        secondary replica in an Always On availability
        group

      --authentication-method
        Specifies the SQL authentication method to use to connect
        to Azure SQL Database. One of:
        ActiveDirectoryDefault, ActiveDirectoryIntegrated,
        ActiveDirectoryPassword,
        ActiveDirectoryInteractive,
        ActiveDirectoryManagedIdentity,
        ActiveDirectoryServicePrincipal, SqlPassword

  -c, --batch-terminator
        Specifies the batch terminator. The default value is GO

  -s, --column-separator
        Specifies the column separator character. Sets the
        SQLCMDCOLSEP variable.

I am liking lower one better as compared to first one.
But still if we want to have flags without spaces , i could do those changes.

cmd/sqlcmd/sqlcmd.go Outdated Show resolved Hide resolved
@stuartpa
Copy link
Collaborator

@stuartpa Are you talking about the description should look like below :

Flags:
-K,--application-intent
Declares the application workload type when connecting to a
server. The only currently supported value is ReadOnly. If
-K is not specified, the sqlcmd utility will not support
connectivity to a secondary replica in an Always On
availability group

      --authentication-method
Specifies the SQL authentication method to use to connect
to Azure SQL Database. One of: ActiveDirectoryDefault,
ActiveDirectoryIntegrated, ActiveDirectoryPassword,
ActiveDirectoryInteractive, ActiveDirectoryManagedIdentity,
ActiveDirectoryServicePrincipal, SqlPassword

-c,--batch-terminator
Specifies the batch terminator. The default value is GO

beacuse right now it looks like this :

Flags:
  -K, --application-intent
        Declares the application workload type when connecting to a
        server. The only currently supported value is
        ReadOnly. If -K is not specified, the sqlcmd
        utility will not support connectivity to a
        secondary replica in an Always On availability
        group

      --authentication-method
        Specifies the SQL authentication method to use to connect
        to Azure SQL Database. One of:
        ActiveDirectoryDefault, ActiveDirectoryIntegrated,
        ActiveDirectoryPassword,
        ActiveDirectoryInteractive,
        ActiveDirectoryManagedIdentity,
        ActiveDirectoryServicePrincipal, SqlPassword

  -c, --batch-terminator
        Specifies the batch terminator. The default value is GO

  -s, --column-separator
        Specifies the column separator character. Sets the
        SQLCMDCOLSEP variable.

I am liking lower one better as compared to first one. But still if we want to have flags without spaces , i could do those changes.

Like the lower one, but back two spaces

cmd/sqlcmd/sqlcmd.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@shueybubbles shueybubbles left a comment

Choose a reason for hiding this comment

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

🕐

Copy link
Collaborator

@shueybubbles shueybubbles left a comment

Choose a reason for hiding this comment

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

:shipit:

@JyotikaGargg JyotikaGargg merged commit d1965d1 into main May 30, 2023
6 checks passed
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.

None yet

3 participants