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.

Sign up for GitHub

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

Jump to bottom

Set deprecation warning on query command #2967

Merged
2 commits merged into from Nov 29, 2019
Merged

Set deprecation warning on query command #2967

2 commits merged into from Nov 29, 2019

Conversation

bronzdoc
Copy link
Member

Description:

We have been discouraging user for the use of the query command for a long time.

Summary:
    Query gem information in local or remote repositories

  Description:
    The query command is the basis for the list and search commands.

    You should really use the list and search commands instead.  This command
    is too hard to use.

It's time to deprecate it.

This will setup a deprecation warning whenever is used.

I will abide by the code of conduct.

@bronzdoc
Copy link
Member Author

@bundlerbot try

@deivid-rodriguez deivid-rodriguez mentioned this pull request Nov 1, 2019
Closed
@deivid-rodriguez
Copy link
Member

@bundlerbot try

ghost pushed a commit that referenced this pull request Nov 19, 2019
@bundlerbot
Try #2967:
27367c8
@bronzdoc
Copy link
Member Author

@deivid-rodriguez I think I need to rebase this

@deivid-rodriguez
Copy link
Member

@bundlerbot try should test the PR against latest master, so in principle I don't think it needs rebasing?

Not sure if you planned to add some tests for the deprecation message.

@ghost
Copy link

ghost commented Nov 19, 2019

try

Build succeeded

  • macos (2.4.x)
  • macos (2.5.x)
  • macos (2.6.x)
  • ruby_master
  • ubuntu (2.3.x, bundler)
  • ubuntu (2.3.x, rubygems)
  • ubuntu (2.4.x, bundler)
  • ubuntu (2.4.x, rubygems)
  • ubuntu (2.5.x, bundler)
  • ubuntu (2.5.x, rubygems)
  • ubuntu (2.6.x, bundler)
  • ubuntu (2.6.x, rubygems)
  • ubuntu_bundler_master (2.6.x)
  • ubuntu_lint
  • ubuntu_rvm (jruby-9.2.9.0, rubygems)
  • ubuntu_rvm (ruby-head, bundler)
  • windows (2.4.x)
  • windows (2.5.x)
  • windows (2.6.x)

@bronzdoc
Copy link
Member Author

bronzdoc commented Nov 19, 2019 •
edited

Your are right @deivid-rodriguez I forgot 🙄

Not sure if you planned to add some tests for the deprecation message.

Was not on my plans 😬 but I could add one 😃

@deivid-rodriguez
Copy link
Member

Up to you, if it's not too hard I think we can add them.

deivid-rodriguez
deivid-rodriguez approved these changes Nov 29, 2019
View reviewed changes
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.

@bronzdoc I mean a test checking that the deprecation text is printed to the screen when the end user uses the command. But it's not big deal, I just tried it manually and it works :)

Something I would like that we improved in the future is to be able to give the user a more specific message.

So, instead of saying:

NOTE: query command is deprecated. It will be removed on or after 2019-12-01

we could say

NOTE: query command is deprecated because of being too hard to use. It will be removed on or after 2019-12-01. Please use the alternative commands `gem search` for searching for remote gems or `gem info` for looking up information for locally installed gems.

But we can improve this in the future.

@bronzdoc
Copy link
Member Author

@deivid-rodriguez since command_manager is the responsible for printing the command deprecation warning I added a test here when I introduced the deprecate_command method.

Said that, I understand what you mean. Maybe we want each command to handle it's own deprecation warning?

Something I would like that we improved in the future is to be able to give the user a more specific message.

That would be nice 👍

@deivid-rodriguez
Copy link
Member

I see.

Said that, I understand what you mean. Maybe we want each command to handle it's own deprecation warning?

If this is easy to do and leads to better testability and similarly complex code, I'm all for it. That said, I'm happy with this PR being merged as is.

@bronzdoc
Copy link
Member Author

bronzdoc commented Nov 29, 2019 •
edited

If this is easy to do and leads to better testability and similarly complex code, I'm all for it. That said, I'm happy with this PR being merged as is.

@deivid-rodriguez I'll look into this and thanks looking at this PR

@bronzdoc
Copy link
Member Author

@bundlerbot r+

ghost pushed a commit that referenced this pull request Nov 29, 2019
@bundlerbot @bronzdoc
Merge #2967
b89d261 
2967: Set deprecation warning on query command r=bronzdoc a=bronzdoc

# Description:
We have been discouraging user for the use of the query command for a long time.
```
Summary:
    Query gem information in local or remote repositories

  Description:
    The query command is the basis for the list and search commands.

    You should really use the list and search commands instead.  This command
    is too hard to use.
```

 It's time to deprecate it.

This will setup a deprecation warning whenever is used.

______________
I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).


Co-authored-by: bronzdoc <lsagastume1990@gmail.com>
@ghost
Copy link

ghost commented Nov 29, 2019

Build succeeded

  • macos (2.4.x)
  • macos (2.5.x)
  • macos (2.6.x)
  • ruby_master
  • ubuntu (2.3.x, bundler)
  • ubuntu (2.3.x, rubygems)
  • ubuntu (2.4.x, bundler)
  • ubuntu (2.4.x, rubygems)
  • ubuntu (2.5.x, bundler)
  • ubuntu (2.5.x, rubygems)
  • ubuntu (2.6.x, bundler)
  • ubuntu (2.6.x, rubygems)
  • ubuntu_bundler_master (2.6.x)
  • ubuntu_lint
  • ubuntu_rvm (jruby-9.2.9.0)
  • ubuntu_rvm (ruby-head)
  • windows (2.4.x)
  • windows (2.5.x)
  • windows (2.6.x)

@ghost ghost merged commit 2e789c4 into master Nov 29, 2019
@ghost ghost deleted the depcreate_query_command branch November 29, 2019 14:19
@deivid-rodriguez deivid-rodriguez mentioned this pull request Dec 3, 2019
Merged
4 tasks
ghost pushed a commit that referenced this pull request Dec 3, 2019
@bundlerbot @deivid-rodriguez
Merge #3014
fbcd299 
3014: Improve deprecations r=bronzdoc a=deivid-rodriguez

# Description:

#2967 deprecated `gem query` but it also deprecated `gem list`, `gem search`, and `gem info`, because they inherit from `gem query`.

This PR undeprecates these commands by no longer inheriting from `QueryCommand`.

It also adds tests to ensure that `gem query` is deprecated, but `gem list`, `gem search`, and `gem info` are not.

Fixes #3008.

# Tasks:

- [x] Describe the problem / feature
- [x] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
ghost pushed a commit that referenced this pull request Dec 4, 2019
@bundlerbot @deivid-rodriguez
Merge #3014
cc8e975 
3014: Improve deprecations r=bronzdoc a=deivid-rodriguez

# Description:

#2967 deprecated `gem query` but it also deprecated `gem list`, `gem search`, and `gem info`, because they inherit from `gem query`.

This PR undeprecates these commands by no longer inheriting from `QueryCommand`.

It also adds tests to ensure that `gem query` is deprecated, but `gem list`, `gem search`, and `gem info` are not.

Fixes #3008.

# Tasks:

- [x] Describe the problem / feature
- [x] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).


Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
@deivid-rodriguez deivid-rodriguez mentioned this pull request Feb 1, 2020
Merged
@Irakaj93 Irakaj93 mentioned this pull request Dec 26, 2020
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deivid-rodriguez deivid-rodriguez deivid-rodriguez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bronzdoc @deivid-rodriguez