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

Make user confs have at leats one : separator #15296

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

AbrilRBS
Copy link
Member

@AbrilRBS AbrilRBS commented Dec 18, 2023

Changelog: Bugfix: Ensure user confs have at least 1 : separator
Changelog: Fix: Fix user confs package scoping when no separator was given
Docs: Omit

Fix comes from trying to fix error messages when they disdnt have a : but were scoped

@AbrilRBS AbrilRBS added this to the 2.0.15 milestone Dec 18, 2023
@AbrilRBS AbrilRBS requested a review from memsharded December 18, 2023 16:24
conans/model/conf.py Outdated Show resolved Hide resolved
conans/test/integration/package_id/compatible_test.py Outdated Show resolved Hide resolved
("user.foo:bar=1", None),
("user.baz=1", "'user.baz' must have at least one ':' separator"),
])
def test_conf_scope_patterns_battery(conf, assert_message):
Copy link
Member Author

Choose a reason for hiding this comment

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

Failing test, fixing it now

@AbrilRBS AbrilRBS marked this pull request as draft December 18, 2023 16:29
@AbrilRBS AbrilRBS marked this pull request as ready for review December 18, 2023 17:10
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.

Looking good, just a couple of details

conans/test/unittests/model/test_conf.py Outdated Show resolved Hide resolved
conans/model/conf.py Outdated Show resolved Hide resolved
@franramirez688 franramirez688 self-assigned this Dec 19, 2023
@franramirez688
Copy link
Contributor

franramirez688 commented Dec 19, 2023

@RubenRBS From my point of view, the changes to be made should be something like this:

diff --git a/conans/model/conf.py b/conans/model/conf.py
index 4a46d91ec..45f1d4000 100644
--- a/conans/model/conf.py
+++ b/conans/model/conf.py
@@ -116,9 +116,9 @@ BUILT_IN_CONFS = {
 BUILT_IN_CONFS = {key: value for key, value in sorted(BUILT_IN_CONFS.items())}


-CORE_CONF_PATTERN = re.compile(r"^core[.:]")
-TOOLS_CONF_PATTERN = re.compile(r"^tools[.:]")
-USER_CONF_PATTERN = re.compile(r"^user[.:]")
+CORE_CONF_PATTERN = re.compile(r"^(core\..+|core):.*")
+TOOLS_CONF_PATTERN = re.compile(r"^(tools\..+|tools):.*")
+USER_CONF_PATTERN = re.compile(r"^(user\..+|user):.*")


 def _is_profile_module(module_name):
@@ -496,8 +496,9 @@ class Conf:
     @staticmethod
     def _check_conf_name(conf):
         if USER_CONF_PATTERN.match(conf) is None and conf not in BUILT_IN_CONFS:
-            raise ConanException(f"[conf] '{conf}' does not exist in configuration list. "
-                                 f" Run 'conan config list' to see all the available confs.")
+            raise ConanException(f"[conf] Either '{conf}' does not exist in configuration list or "
+                                 f"the conf format introduced is not valid. Run 'conan config list' "
+                                 f"to see all the available confs.")

I think that this should cover everything 😁

UPDATED: I just updated the pattern 😅

@czoido czoido merged commit ad12191 into conan-io:release/2.0 Dec 19, 2023
2 checks passed
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.

4 participants