Skip to content

Conversation

hezhizhen
Copy link
Contributor

Summary

  1. Use imperative sentences for command descriptions
  2. Use exported functions for commands added to the root

How was it tested?

@hezhizhen hezhizhen marked this pull request as ready for review March 30, 2023 06:13
Copy link
Contributor

@ipince ipince left a comment

Choose a reason for hiding this comment

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

Thanks!

@savil
Copy link
Collaborator

savil commented Mar 31, 2023

Use exported functions for commands added to the root

why is this needed?

@hezhizhen
Copy link
Contributor Author

hezhizhen commented Mar 31, 2023

Use exported functions for commands added to the root

why is this needed?

Not needed, because all functions are in the same package.

But most of functions for commands added to the root are exported and sub-commands for these commands are unexported. So I think it's better to follow the pattern.

@savil What do you think?

@savil
Copy link
Collaborator

savil commented Mar 31, 2023

Oh I see. I think that may be an oversight. In general, we should bias towards not visible unless we intentionally want to export the function.

So, could we instead go the other way, and make the exported Command Functions be internal? I don't see a good reason for why they need to be exported.

@hezhizhen
Copy link
Contributor Author

Oh I see. I think that may be an oversight. In general, we should bias towards not visible unless we intentionally want to export the function.

So, could we instead go the other way, and make the exported Command Functions be internal? I don't see a good reason for why they need to be exported.

Okay, I've changed all functions to be internal.

@savil
Copy link
Collaborator

savil commented Mar 31, 2023

@hezhizhen thank you! for your PR and openness to modifying it.

@savil savil merged commit 4c486c6 into jetify-com:main Mar 31, 2023
@hezhizhen hezhizhen deleted the description branch August 9, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants