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

Omit deprecated commands from command help output #4023

Merged

Conversation

landongrindheim
Copy link
Contributor

@landongrindheim landongrindheim commented Oct 20, 2020

What was the end-user or developer problem that led to this PR?

gem query was deprecated in rubygems 3.2. It was noted in #3984 that there should be a deprecation warning in the CLI (done in #4021) and a message in the guides.

A PR was opened to address the message (rubygems/guides#270) in Guides, but it was pointed out that the file being changed is auto-generated. This change leans into that auto-generation and should make rubygems/guides#270 redundant.

Closes #3984

What is your fix for the problem, implemented in this PR?

Add the deprecation note to the command's summary so that it can be relied upon for auto-generating the documentation. This note will display near the heading in the guides.

Make sure he following tasks are checked

This has been specific to Commands::QueryCommand for at least a while
now, though its implementation is in the mixin. By moving it out of the
mixin, we ensure the description is relevant to the command at hand.
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

I was reviewing this, but then I had a maybe better idea. What if, instead, we change the script in https://github.com/rubygems/guides/ to ignore deprecated commands and don't list them in the command reference section at all. I think, once we deprecate a command, it's no worth documenting it at all?

Thoughts?

lib/rubygems/commands/query_command.rb Outdated Show resolved Hide resolved
lib/rubygems/commands/query_command.rb Outdated Show resolved Hide resolved
@landongrindheim
Copy link
Contributor Author

What if, instead, we change the script in https://github.com/rubygems/guides/ to ignore deprecated commands and don't list them in the command reference section at all. I think, once we deprecate a command, it's no worth documenting it at all?

@deivid-rodriguez This makes sense to me -- why hold the user's hand when the intent is to get them to stop using the functionality? I've poked around online for approaches to documenting deprecated functionality. This is what I've seen so far:

You're doing the heavy lifting here, so I think you should feel free to choose the direction you'd like this to go 🙂

If this PR can be helpful, (say, adding **deprecated** to the summary instead of the newline which breaks functionality with gem help), please let me know and I'll adjust accordingly.

@deivid-rodriguez
Copy link
Member

Thanks for the research! Yeah, that's exactly what I was thinking. If we intend users to stop using a command, not documenting it sounds like a good step in that direction.

The deprecation is removed altogether in favour of not rendering documentation for deprecated commands

I'd go with this one.

Should gem help commands be consistent with the guides and skip deprecated commands?

Yes, that's sounds like the right way forward to me.

Should there be mention of the deprecation anywhere other than in CLI output upon use?

I don't think so, just warning on usage and recommending alternatives seems fine to me.

@landongrindheim
Copy link
Contributor Author

landongrindheim commented Oct 22, 2020

@deivid-rodriguez What do you think of this approach?

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

The approach looks good! Only added a couple of minor comments.

Could you squash everything into two commits, one that moves the description to the query command, and the other one that implements "removing" deprecated commands from help output?

lib/rubygems/commands/query_command.rb Outdated Show resolved Hide resolved
lib/rubygems/commands/query_command.rb Show resolved Hide resolved
This change came about through discussion with David Rodgriguez. We've
been deliberating whether to document the deprecated status of commands.
I looked for examples in the OSS community and found that there's a lot
of inconsistency around documenting deprecated items. Ruby does and
doesn't do it. Rails seems to do it. Elixir seems to do it. It's not a
settled matter.

Ultimately, the decision was to remove the deprecated commands from the
`help commands` command.
@landongrindheim landongrindheim force-pushed the move-description-into-query-command branch from 11d262c to 7b3012d Compare October 22, 2020 18:06
@deivid-rodriguez deivid-rodriguez changed the title Move description into query command Omit deprecated commands from command help output Oct 22, 2020
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks so much for this fix and for your patience with my picky reviews 😃

Copy link
Member

@hsbt hsbt left a comment

Choose a reason for hiding this comment

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

It's nice feature.

@hsbt hsbt merged commit a3427c7 into rubygems:master Oct 22, 2020
@hsbt hsbt added this to the RubyGems 3.2.0/Bundler 2.2.0 milestone Oct 22, 2020
deivid-rodriguez pushed a commit that referenced this pull request Dec 7, 2020
…uery-command

Omit deprecated commands from command help output

(cherry picked from commit a3427c7)
deivid-rodriguez pushed a commit that referenced this pull request Dec 7, 2020
…uery-command

Omit deprecated commands from command help output

(cherry picked from commit a3427c7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

query command deprecation clarification
4 participants