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

Enforce required, optional, and rest positional arguments start with an uppercase and end with a period. #11285

Merged
merged 7 commits into from Dec 15, 2023

Conversation

drbrain
Copy link
Contributor

@drbrain drbrain commented Dec 11, 2023

Description

This updates all the positional arguments (except with --features=dataframe or --features=extra) to start with an uppercase letter and end with a period.

Part of #5066, specifically this comment

Some arguments had example data removed from them because it also appears in the examples.

There are other inconsistencies in positional arguments I noticed while making the tests pass which I will bring up in #5066.

User-Facing Changes

Positional arguments are now consistent

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

Automatic documentation updates

@fdncred
Copy link
Collaborator

fdncred commented Dec 11, 2023

wow, that's a lot of changes. thanks for all the hard work. i'm especially glad to enforce consistency with tests. i'm a little bit nervous on landing this one before our release tomorrow since it's so big.

@drbrain
Copy link
Contributor Author

drbrain commented Dec 11, 2023

i'm a little bit nervous on landing this one before our release tomorrow since it's so big.

I'm in no rush to merge

@ghost
Copy link

ghost commented Dec 12, 2023

Hi @drbrain 👋, where does one find the toolkit used for these updates?

Copy link
Collaborator

@WindSoilder WindSoilder left a comment

Choose a reason for hiding this comment

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

Thanks! I really love these two added tests: arguments_start_uppercase, arguments_end_period

@WindSoilder WindSoilder merged commit 5b01685 into nushell:main Dec 15, 2023
19 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Dec 15, 2023

@drbrain I'm wondering if all parameters are checked for uppercase and period. I just landed a PR that passed the test but didn't have a period in the switches. https://github.com/nushell/nushell/pull/11324/files#diff-61e5ce298fd8925da5182d9bddcf1d27568c5a74ece04c9f9b8a699688ec5bbcR481-R484

@Deebster
Copy link

Deebster commented Feb 7, 2024

This change seems to have a few problems, and does not match with what was agreed in #5066:

Argument desc: begin by a capital and do not end with a dot

However, this PR enforces the opposite, and now parameters are inconsistent with flags, even though they're both arguments (in the argv sense).

Also, now the (optional) annotation looks like an error since it is lowercase but appears as a new sentence.

For example, in hide --help we see this:

Flags:
  -h, --help - Display the help message for this command

Parameters:
  module <string>: Module or module file.
  members <any>: Which members of the module to import. (optional)

Instead of the cleaner and more consistent:

Flags:
  -h, --help - Display the help message for this command

Parameters:
  module <string>: Module or module file
  members <any>: Which members of the module to import (optional)

dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
…an uppercase and end with a period. (nushell#11285)

# Description

This updates all the positional arguments (except with
`--features=dataframe` or `--features=extra`) to start with an uppercase
letter and end with a period.

Part of nushell#5066, specifically [this
comment](/nushell/issues/5066#issuecomment-1421528910)

Some arguments had example data removed from them because it also
appears in the examples.

There are other inconsistencies in positional arguments I noticed while
making the tests pass which I will bring up in nushell#5066.

# User-Facing Changes

Positional arguments are now consistent

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

Automatic documentation updates
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

4 participants