Skip to content

Conversation

@jonathanc-n
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Implements the changes from the review in #12861 by @Omega359

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added documentation Improvements or additions to documentation functions Changes to functions implementation labels Oct 11, 2024
@Omega359
Copy link
Contributor

LGTM, thx!

+-------------------------------------------------+
```"#)
.with_argument("expression", "Expression to operate on. Can be a constant, column, or function, and any combination of arithmetic operators.")
.with_standard_argument("expression", "The")
Copy link
Member

Choose a reason for hiding this comment

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

curious, what does "The" stand for?

Copy link
Contributor

Choose a reason for hiding this comment

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

/// Add a standard "expression" argument to the documentation

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the link. "The" still looks weird to me. I guess it gets concatenated with something else.

if i follow the link correctly, this gets passed as expression_type argument, but "The" does not look like expression type.

Could we have an overload so that "The" is not needed?
eg with_standard_sole_argument could produce "The expression to operate on. Can be a constant, column, or function, and any combination of operators." without requiring "The" from the caller.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it really does look strange. .with_standard_argument("expression", None) / with_standard_argument("expression", Some("String")) ?

@jonathanc-n
Copy link
Contributor Author

@findepi Does it look good?

@findepi
Copy link
Member

findepi commented Oct 14, 2024

Does it look good?

per #12880 (comment), i would prefer to have these "The" removed. Would you like to add a commit for this?

@Omega359
Copy link
Contributor

Does it look good?

per #12880 (comment), i would prefer to have these "The" removed. Would you like to add a commit for this?

I think we may need to update the with_standard_argument in some manner or have another method to clean that up. I was toying with either "" == "Expression to operate on....", Change the arg to an option, or `with_standard_argument_<something_here>(..)

@alamb
Copy link
Contributor

alamb commented Oct 15, 2024

Thank you @jonathanc-n and @Omega359 and @findepi

@alamb
Copy link
Contributor

alamb commented Oct 16, 2024

I think we can improve the argument handling as follow on PRs

@alamb alamb merged commit 5d6254c into apache:main Oct 16, 2024
@jonathanc-n jonathanc-n deleted the fix-aggregate-docs branch November 1, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants