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

Add option --deplist for repoquery #621

Closed
wants to merge 2 commits into from

Conversation

j-mracek
Copy link
Member

It also allows to run it as command 'dnf deplist ' - yum compatibility.

I will provide test for CI-dnf-stack

Copy link

@mhatina mhatina left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@mhatina
Copy link

mhatina commented Oct 3, 2016

Waiting for tests to be merged (rpm-software-management/ci-dnf-stack#183).

@@ -90,7 +90,7 @@ class RepoQueryCommand(commands.Command):
"""A class containing methods needed by the cli to execute the repoquery command.
"""

aliases = ('repoquery',)
aliases = ('repoquery', 'deplist')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not this belong to yum-dnf wrapper @jsilhan?

Copy link
Contributor

Choose a reason for hiding this comment

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

@j-mracek , has yum repoquery really --deplist option? If so then it would be easier to have it here otherwise it should be rather another command file. I don't know if architecture of yum wrapper allows adding custom commands. Does it, @mhatina ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jsilhan repoquery (yum-utils) do not have option --deplist.
The solution here allows hidden compatibility with "yum deplist", but it has all functionality of repoquery therefore it allows all options for package filtrating, but no additional lines of code were required. Somehow the new option for repoguery sounds better to me (with more functionality) than new command (require more code). The solution still keeps 100% compatibility with 'yum deplist'.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mhatina Please the PR requires your opinion from jsilhan question.

Copy link

Choose a reason for hiding this comment

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

@jsilhan It does now. (Sorry for late response, I have to modify my filters in gmail)

Copy link

Choose a reason for hiding this comment

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

@jsilhan @j-mracek Do we have a solution for this? Will it be implemented using yum wrapper?

@@ -120,6 +120,8 @@ def set_argparser(parser):
help=_("check non-explicit dependencies (files and Provides); default"))
whatrequiresform.add_argument("--exactdeps", action="store_true",
help=_('check dependencies exactly as given, opposite of --alldeps'))
parser.add_argument('--deplist', action='store_true',
help=_("list of all dependencies and what packages it provides"))
Copy link
Member

Choose a reason for hiding this comment

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

[flake8]

  • [E501] line too long (93 > 79 characters)

@j-mracek j-mracek force-pushed the deplist branch 2 times, most recently from 1827871 to 393b041 Compare November 8, 2016 14:00
===========================
No ``deplist`` command
===========================

Copy link
Member Author

Choose a reason for hiding this comment

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

Here I am not sure if it should be deleted. Command yum deplist will works but not dnf deplist. For dnf you have to use dnf repoquery --deplist. @jsilhan Is that what was supposed to? And how to handle descriptions in cli_vs_yum.rst ?

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR cli_vs_yum is documentation of differences of upstream DNF project and yum (yum-deprecated). So in this case it should be there as a change although there should be some .. seealso:: mentioning the command exists in yum-dnf when /usr/bin/yum is executed.

@j-mracek
Copy link
Member Author

j-mracek commented Nov 9, 2016

test this please

@mhatina
Copy link

mhatina commented Nov 11, 2016

@dnf-bot retest this please

@j-mracek
Copy link
Member Author

test this please

1 similar comment
@j-mracek
Copy link
Member Author

test this please

@dnf-bot
Copy link
Member

dnf-bot commented Nov 21, 2016

📌 Commit de287cf has been approved by mhatina

@dnf-bot
Copy link
Member

dnf-bot commented Nov 21, 2016

⌛ Testing commit de287cf with merge 03277ce...

dnf-bot pushed a commit that referenced this pull request Nov 21, 2016
dnf-bot pushed a commit that referenced this pull request Nov 21, 2016
@dnf-bot
Copy link
Member

dnf-bot commented Nov 21, 2016

☀️ Test successful - status-jenkins
Approved by: mhatina
Pushing 03277ce to master...

@dnf-bot dnf-bot closed this in fea4c74 Nov 21, 2016
"""

aliases = ('deplist',)
summary = _("List a package's dependencies")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not grammatical. Should be "List packages' dependencies".

Copy link

Choose a reason for hiding this comment

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

It is grammatically correct. It just lists dependencies of one package and what you suggested lists dependencies of more packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Singular: "List package's dependencies", plural: "List packages' dependencies". Pick one.


class DeplistCommand(RepoQueryCommand):
"""
some comment about command
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link

Choose a reason for hiding this comment

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

Yes, I missed that in review.

@@ -120,6 +120,8 @@ def set_argparser(parser):
help=_("check non-explicit dependencies (files and Provides); default"))
whatrequiresform.add_argument("--exactdeps", action="store_true",
help=_('check dependencies exactly as given, opposite of --alldeps'))
parser.add_argument('--deplist', action='store_true', help=_(
"list of all dependencies and what packages it provides"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence makes not sense ;(

Copy link

@mhatina mhatina Nov 21, 2016

Choose a reason for hiding this comment

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

It does make sense. It lists all dependencies and for each dependency lists packages that provide given dependency.

Maybe it could be changed, so it is easily understandable.

Copy link
Contributor

Choose a reason for hiding this comment

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

"It" refers to "list" ("list" is the subject of the sentence). But a "list" does not "provide" packages. I'm not sure what exactly the sentence was supposed to convey, so I don't know how to fix it, but it needs to be changed.

@mluscon mluscon reopened this Nov 21, 2016
@mluscon
Copy link
Contributor

mluscon commented Nov 21, 2016

@j-mracek, take a second look please.

@j-mracek
Copy link
Member Author

@mhatina @keszybz Here is the patch that change descriptions. Thank you very much for notes the problem.

@mhatina
Copy link

mhatina commented Nov 22, 2016

@dnf-bot r+

@dnf-bot
Copy link
Member

dnf-bot commented Nov 22, 2016

📌 Commit 22a6c17 has been approved by mhatina

@dnf-bot
Copy link
Member

dnf-bot commented Nov 22, 2016

⌛ Testing commit 22a6c17 with merge 88a8e46...

dnf-bot pushed a commit that referenced this pull request Nov 22, 2016
@dnf-bot
Copy link
Member

dnf-bot commented Nov 22, 2016

💔 Test failed - status-jenkins

@ignatenkobrain
Copy link
Contributor

@dnf-bot retry

@dnf-bot
Copy link
Member

dnf-bot commented Nov 22, 2016

⌛ Testing commit 22a6c17 with merge 7b7950a...

dnf-bot pushed a commit that referenced this pull request Nov 22, 2016
@dnf-bot
Copy link
Member

dnf-bot commented Nov 22, 2016

💔 Test failed - status-jenkins

@@ -121,7 +121,7 @@ def set_argparser(parser):
whatrequiresform.add_argument("--exactdeps", action="store_true",
help=_('check dependencies exactly as given, opposite of --alldeps'))
parser.add_argument('--deplist', action='store_true', help=_(
"list of all dependencies and what packages it provides"))
"show a list of all dependencies and what packages provide it"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This version is better, thanks. "provide them" would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

@keszybz Thanks for corrections, I just overlooked the text (too much inspiration from yum)

@j-mracek
Copy link
Member Author

@dnf-bot r+

@dnf-bot
Copy link
Member

dnf-bot commented Nov 22, 2016

📌 Commit e53c234 has been approved by j-mracek

@dnf-bot
Copy link
Member

dnf-bot commented Nov 22, 2016

⌛ Testing commit e53c234 with merge d6efe72...

@dnf-bot
Copy link
Member

dnf-bot commented Nov 22, 2016

☀️ Test successful - status-jenkins
Approved by: j-mracek
Pushing d6efe72 to master...

@dnf-bot dnf-bot closed this in d6efe72 Nov 22, 2016
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.

None yet

7 participants