Feature: Parse reference attributes from --ref in workspace add (#20261) - #20269
Conversation
memsharded
left a comment
There was a problem hiding this comment.
Some unit test would be necessary for this feature, please try to add some (likely to go in the integration folder, the easier is using the TestClient helper), and ask for guidance if necessary.
Many thanks for your contribution!
| group.add_argument("-nr", "--no-remote", action="store_true", | ||
| help='Do not use remote, resolve exclusively in the cache') | ||
| args = parser.parse_args(*args) | ||
| if args.path and args.ref: |
There was a problem hiding this comment.
Why is this check changed? Seems mostly unrelated to the parsing of --ref
| ref = conan_api.workspace.add(path, | ||
| args.name, args.version, args.user, args.channel, | ||
| cwd, args.output_folder, remotes=remotes) | ||
| elif not path: |
There was a problem hiding this comment.
Same above, I am not sure why this check was changed.
|
|
||
| if args.ref: | ||
| # TODO: Use path here to open in this path | ||
| if '/' in args.ref or '@' in args.ref: |
There was a problem hiding this comment.
The --ref should be parsed in any case, it is not intended that --ref can be an incomplete reference with only the name, for that purpose the individual arguments are there.
Probably the RecipeReference validation could be used?
Also, once the RecipeReference is loaded, it doesn't make sense to mix it with individual arguments, it should be either --ref or the individual arguments, but not both. Maybe for that case, an extra check could be added, but not necessary in this PR, this PR can keep focused on the parsing.
|
Conan 2.32 release is approaching, if we want this in, it would be necessary to address these things this week. Otherwise, it will have to wait until 2.33 next month. Thanks! |
|
hey @memsharded Thanks for the review. The mutual exclusion test was stripped out in an earlier commit, but now it's back in the latest version, also the things u said are taken care of now inside the new commit, like parsing and more. The PR now only deals with parsing of --ref without any other changes.
|
memsharded
left a comment
There was a problem hiding this comment.
Please merge from develop2 to avoid conflicts
| # --- PARSING LOGIC | ||
| name = args.name | ||
| version = args.version | ||
| user = args.user | ||
| channel = args.channel | ||
|
|
||
| if args.ref: | ||
| # TODO: Use path here to open in this path | ||
| from conan.api.model import RecipeReference | ||
| try: | ||
| parsed_ref = RecipeReference.loads(args.ref) | ||
| name = parsed_ref.name | ||
| version = parsed_ref.version | ||
| user = parsed_ref.user | ||
| channel = parsed_ref.channel |
There was a problem hiding this comment.
This still allows definition of both arguments, silently ignoring some arguments.
When args.ref is defined, it means args.name/version/user/channel cannot be defined, and defining both should raise an error, that would be better.
| client.run("export . --user=company") | ||
|
|
||
| client.run("workspace init .") | ||
| client.run("workspace add --ref mypkg/1.0@company --version=2.0") |
There was a problem hiding this comment.
This should raise an error, because of defining both arguments.
| """ | ||
| Add packages to current workspace | ||
| """ |
There was a problem hiding this comment.
If something is just format/style, better not change it
| subparser.add_argument("-of", "--output-folder", | ||
| help='The root output folder for generated and build files') | ||
| subparser.add_argument("--ref", help="Open and add this reference (format: name/version@user/channel)") | ||
| subparser.add_argument("-of", "--output-folder", help='The root output folder for generated and build files') |
There was a problem hiding this comment.
same, avoid formatting changes
| help='Do not use remote, resolve exclusively in the cache') | ||
| args = parser.parse_args(*args) | ||
|
|
||
| # keeping the check |
There was a problem hiding this comment.
remove these lines, nothing changed here
…n-io#20261) - Removed mutual exclusion check for path and --ref - Added parsing of --ref to extract name, version, user, channel - Uses RecipeReference.loads() for robust parsing - Added clear error message for invalid reference format fixes conan-io#20261
5abd67b to
750a4ef
Compare
|
Hello @memsharded , I've cleaned up the branch history and updated the implementation. Only the integration tests and the expected workspace --ref parsing change are included in the most recent commit. Thank you! here it is : omsadegaonkar@750a4ef |
|
Merged, will be in 2.32, thanks for the contribution! |

Changelog: Feature: Parse reference attributes from
--refinworkspace add.Docs: Omit
fixes #20261