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

Start testing some features and changes for the cli in Conan V2 #7170

Closed
wants to merge 52 commits into from

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Jun 9, 2020

Changelog: Feature: Draft to start testing some features that are being discussed for Conan CLI 2.0.
Docs: https://github.com/conan-io/docs/pull/XXXX

Define env var CONAN_V2_CLI to use.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Some very quick comments.

conans/client/conan_api_v2.py Outdated Show resolved Hide resolved
conans/client/conan_api_v2.py Outdated Show resolved Hide resolved
conans/client/conan_api_v2.py Outdated Show resolved Hide resolved
conans/client/outputers/__init__.py Outdated Show resolved Hide resolved
conans/client/outputers/cli.py Outdated Show resolved Hide resolved
conans/client/cmd/search_v2.py Outdated Show resolved Hide resolved
@jgsogo jgsogo added this to the 1.27 milestone Jun 10, 2020
conans/cli/command.py Outdated Show resolved Hide resolved
args = parser.parse_args(*args)

try:
info = self._conan.search_recipes(args.query, remote_pattern=args.remote,
Copy link
Member

Choose a reason for hiding this comment

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

Different api calls for remotes and cache? Otherwise the "exclusive" argument check needs to be duplicated inside the api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that maybe we could allow searching in the remote and the cache at the same time. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Might be possible, indeed. Lets discuss it with the team.

try:
info = self._conan.search_recipes(args.query, remote_pattern=args.remote,
local_cache=args.cache)
except ConanException as exc:
Copy link
Member

Choose a reason for hiding this comment

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

This repeated pattern everywhere is ugly. I think we should try to avoid it when possible, and extract the behavior to a common place when not possible.
I would remove it at the moment, and leave a flow without try-except.

from conans.search.search import (search_recipes)


class Search(object):
Copy link
Member

Choose a reason for hiding this comment

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

An object, without state, is probably better a function, you can pass the app as argument, it will have the collaborators inside.

conans/client/api/conan_api.py Outdated Show resolved Hide resolved
conans/client/api/conan_api.py Outdated Show resolved Hide resolved
@jgsogo jgsogo mentioned this pull request Jun 11, 2020
5 tasks
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I haven't cloned this branch, so don't take too seriously these comments

  • if formatters are so coupled with the commands, probably they could be just methods. And I would prefer to add them in the same file as the command itself, I think it simplifies distribution.

  • I think those formatters doesn't belong to the conans.client module, but to conans.cli:

  • I would return the data structure from the command and let the output handle it

These suggestions will turn the command definition into something like:

@conan_command(group="Misc commands", cli=output_help_cli, json=output_help_json,...)
def help(*args, **kwargs):
   ...
   return help_output

def output_help_cli(out, help_output, *args, **kwargs):
    ...

There can be other strategies to associate output to commands (I'm not sure if those output can be reused by different commands)

conans/cli/command.py Outdated Show resolved Hide resolved
conans/cli/command.py Outdated Show resolved Hide resolved
conans/cli/command.py Outdated Show resolved Hide resolved
conans/cli/command.py Outdated Show resolved Hide resolved
conans/cli/command.py Outdated Show resolved Hide resolved
conans/cli/commands/dig.py Outdated Show resolved Hide resolved
czoido and others added 8 commits June 23, 2020 16:23
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Too many things in one PR. Lets split it in 2 or more PRs:

  • One PR first with the infrastructure for commands mgmt. Can implement just the "search" method with a hardcoded result in 1 line, empty "conan_api", 1 formatter
  • After that is merged, it would be nice to proceed one command at a time, and a lot of the internal functionality is worth revisiting. In that case, it could be separate in one PR for the interface (cli, conan_api), then another for the internal.
  • Also the internal can be duplicated if modifying the existing code is too risky and could break.

@memsharded memsharded modified the milestones: 1.27, 1.28 Jun 28, 2020
@czoido czoido mentioned this pull request Jun 30, 2020
5 tasks
@czoido czoido removed this from the 1.28 milestone Jul 15, 2020
@czoido
Copy link
Contributor Author

czoido commented Jul 15, 2020

Replaced by: #7278 and upcoming PR

@czoido czoido closed this Jul 15, 2020
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

3 participants