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

Custom command attributes #14906

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

Custom command attributes #14906

wants to merge 23 commits into from

Conversation

Bahex
Copy link
Contributor

@Bahex Bahex commented Jan 24, 2025

Description

Add custom command attributes.

  • Attributes are placed before a command definition and start with a @ character.
  • Attribute invocations consist of const command call. The command's name must start with "attr ", but this prefix is not used in the invocation.
    • A command named attr env is invoked as an attribute as @env
  • Several built-in attribute commands are provided as part of this PR
    • attr example: Attaches an example to the commands help text
      # Double numbers
      @example "double an int"  r#'5 | double'#   --result 10
      @example "double a float" r#'0.5 | double'# --result 1.0
      def double []: [number -> number] {
          $in * 2
      }
    • attr env: Equivalent to using def --env
    • attr wrapped: Equivalent to using def --wrapped
    • few others to be added
  • If an attribute has no internal/special purpose, it's stored as command metadata that can be obtained with scope commands.
    • This allows having attributes like @test which can be used by test runners.
  • This PR also makes echo const. Purpose of this is to allow custom attribute definitions using alias. As there is currently no way to define const commands, this is intended to provide a stopgap solution.
    alias "attr test" = echo
    
    @test
    def my-test-command [] {}
    scope commands | where attributes.name has "test" will return all commands that have the test attribute.
  • Used the @example attribute for std examples.

User-Facing Changes

Users can add examples to their own command definitions, and add other arbitrary attributes.

Tests + Formatting

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

After Submitting

Add documentation about the attribute syntax and built-in attributes

@fdncred
Copy link
Collaborator

fdncred commented Jan 24, 2025

This is so cool! 😍

I'm going through some of the script changes and noticing some inconsistencies. It could just be how the scripts are written.

There's an extra space where the arrows are.

Seems like all custom commands have the extra space on the last example
image

I noticed that how you use the stdlib can affect the help

I don't think this is a problem but something for all of us to be aware of. Definitely not a problem with your code.
image

image
We're a little bit inconsistent. Sometimes you have to use blah * like above and sometimes you have to use blah like in the dt script. But that's not a problem with this PR either.

Should we require a description?

The middle example looks off because of not having a description. Seems like we should require the description and the code but not the result.
image

I also have some nitpicks of the stdlib but nothing to do with your code. For instance, we shouldn't artificially wrap description text and just let the terminal do the wrapping. Again, not related to your PR.

I think these generally look fantastic! Great job!

@fdncred
Copy link
Collaborator

fdncred commented Jan 24, 2025

Looks better with descriptions. There's still an extra line at the end of examples. It's not a deal-breaker and may be unrelated.
image

@Bahex
Copy link
Contributor Author

Bahex commented Jan 24, 2025

@fdncred I observed the extra line in built-in commands' help texts too. Seems like it was a deliberate choice:

long_desc.push('\n');

@fdncred
Copy link
Collaborator

fdncred commented Jan 24, 2025

@fdncred I observed the extra line in built-in commands' help texts too. Seems like it was a deliberate choice:

long_desc.push('\n');

ok, I don't like it. 😆 Maybe I'll put to get a PR to remove it.

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