Add more meaninful error when parsing conanfile reference (closes #3204) #3410

jgsogo · 2018-08-28T09:26:43Z

Refer to the issue that supports this Pull Request. Criptic exception if float used for version in conanfile.py #3204
If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
I've read the Contributing guide.
I've followed the PEP8 style guides for Python code.

Handles error described in #3204 to provide a more meaninful error:

ERROR: '1.0' is not an string

Changelog: Fix: Improved error message when parsing a bad conanfile reference.

jgsogo · 2018-08-28T09:28:28Z

conans/model/ref.py

@@ -4,11 +4,23 @@
 from conans.model.version import Version


+# TODO: Move to some tools module


Let me know if I leave this code here or move to conans/utils module.... or maybe use a six approach:

from six import string_types isinstance(s, string_types)

Yes, use six tools, please.

jgsogo · 2018-08-28T09:29:48Z

conans/model/ref.py

 class ConanName(object):
    _max_chars = 50
    _min_chars = 2
    _validation_pattern = re.compile("^[a-zA-Z0-9_][a-zA-Z0-9_\+\.-]{%s,%s}$"
-                                     % (_min_chars - 1, _max_chars))
+                                     % (_min_chars - 1, _max_chars - 1))


This is breaking! Should we change _max_chars = 51 too?

Could you add some test checking the length limit of the names (or add a new one checking the upper limit)?

There are tests to check upper and lower limits here and here, maybe in order to be backwards compatible we can make _max_chars = 51 (or greater) and be sure that it is checked and used in strings with this value.

I would say yes, we should raise it to 51

jgsogo · 2018-08-28T09:31:51Z

conans/model/ref.py

        if name == "*":
            return
        if ConanName._validation_pattern.match(name) is None:
            if version and name.startswith("[") and name.endswith("]"):
+                # TODO: Check value between brackets


Leave it as TODO or check version ranges according to semver as documented (https://docs.conan.io/en/latest/mastering/version_ranges.html)?

Could be done in a different PR, not urgent, leave it.

jgsogo · 2018-08-28T09:38:44Z

Related stuff (maybe for a different PR):

validate_user (

conan/conans/model/ref.py

Line 29 in 05d9ca8

def validate_user(username):

) is used only in Username class (

conan/conans/model/username.py

Line 11 in 05d9ca8

ConanName.validate_user(name)

) to validate the username string for the repos, so it is not related to conanfile reference and maybe it should have different requirements. Make it explicit? Document function? Move validation to Username class? ...?

Work ongoing: #3444

memsharded

I don't think this PR is related to #3204.

Not sure if this is the right approach. The original issue is about the version field in conanfile.py, not about the allowed values in ConanFileReference. I would add a check before it is passed to this ConanFileReference class, not such extensive checking inside ConanFileReference. Just an opinion, need to check further.

memsharded · 2018-08-28T09:44:17Z

conans/model/ref.py

@@ -4,11 +4,23 @@
 from conans.model.version import Version


+# TODO: Move to some tools module


Yes, use six tools, please.

jgsogo · 2018-08-29T09:27:59Z

I think that it is ok to centralize here all these checks, but I agree that the message can be more explicit so the user knows where the error comes from. Take a look to the last commit, now the message for the error reported by @ovidiub13 would be:

ERROR: Value provided for package version, '1.0' (type float), is not a string

memsharded · 2018-09-04T22:02:02Z

conans/model/ref.py

-        ConanName.validate_name(user)
-        ConanName.validate_name(channel)
+        message = "Value provided for {item}, '{{value}}' (type {{type}}), {{reason}}"
+        ConanName.validate_name(name, message=message.format(item="package name"))


Do not format every argument (this operation might be done lots of times), for every new ConanFileReference you are formating 4 strings. I know it seems little, but we need to try to keep performance and running time low.

Ok, then I'll go for a different approach

memsharded · 2018-09-04T22:06:26Z

conans/model/ref.py

-            ConanName.invalid_name_message(username)
+    def validate_string(value, message=None):
+        """Check for string"""
+        if not isinstance(value, string_types):


I don't like this checking for string, too defensive. All the fields of ConanFileReference are strings, by definition.

This issue started because we want to provide a more meaninful message for a version = 1.0 (number) inside a conanfile.py... and we didn't want to make an implicit conversion to string. So I think we do need to check for string type (or handle the exception if the regex fails).

lasote · 2018-09-06T06:13:42Z

conans/model/ref.py

 class ConanName(object):
    _max_chars = 50
    _min_chars = 2
    _validation_pattern = re.compile("^[a-zA-Z0-9_][a-zA-Z0-9_\+\.-]{%s,%s}$"
-                                     % (_min_chars - 1, _max_chars))
+                                     % (_min_chars - 1, _max_chars - 1))


I would say yes, we should raise it to 51

lasote · 2018-09-06T06:15:51Z

conans/model/ref.py

        if name == "*":
            return
        if ConanName._validation_pattern.match(name) is None:
            if version and name.startswith("[") and name.endswith("]"):
+                # TODO: Check value between brackets


Could be done in a different PR, not urgent, leave it.

@lasote

…an-io#3204) (conan-io#3410) * check that parameters are string before applying regex match * add tests related to ConanName * typo * update docs related to function * use six to check for string types * use a custom message to validate each field * refactor tests to validate custom messages * fix tests: message is more explicit * given conan-io#3464 this is dead code * format error message only if we are actually going to use it * minor changes: address @lasote review

jgsogo added 4 commits August 28, 2018 11:02

check that parameters are string before applying regex match

3c3b6e8

add tests related to ConanName

161e1f2

typo

f3d6f33

update docs related to function

8133446

jgsogo requested a review from lasote August 28, 2018 09:26

ghost assigned jgsogo Aug 28, 2018

ghost added the stage: review label Aug 28, 2018

jgsogo commented Aug 28, 2018

View reviewed changes

memsharded reviewed Aug 28, 2018

View reviewed changes

use six to check for string types

9f48b5d

lasote added this to the 1.8 milestone Aug 29, 2018

jgsogo added 2 commits August 29, 2018 11:17

use a custom message to validate each field

c933ec0

refactor tests to validate custom messages

46df758

fix tests: message is more explicit

d16c6da

jgsogo changed the title ~~[WIP] Add more meaninful error when parsing conanfile reference (closes #3204)~~ Add more meaninful error when parsing conanfile reference (closes #3204) Aug 31, 2018

jgsogo added 2 commits September 4, 2018 17:09

Merge branch 'develop' into issue/3204

62f771e

given conan-io#3464 this is dead code

7dc4321

memsharded reviewed Sep 4, 2018

View reviewed changes

format error message only if we are actually going to use it

672b460

lasote suggested changes Sep 6, 2018

View reviewed changes

minor changes: address @lasote review

98ad177

memsharded approved these changes Sep 18, 2018

View reviewed changes

memsharded assigned lasote and unassigned jgsogo Sep 18, 2018

lasote approved these changes Sep 20, 2018

View reviewed changes

lasote merged commit d3f6e09 into conan-io:develop Sep 20, 2018

ghost removed the stage: review label Sep 20, 2018

jgsogo deleted the issue/3204 branch September 26, 2018 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more meaninful error when parsing conanfile reference (closes #3204) #3410

Add more meaninful error when parsing conanfile reference (closes #3204) #3410

jgsogo commented Aug 28, 2018 •

edited by lasote

Loading

jgsogo Aug 28, 2018

memsharded Aug 28, 2018

jgsogo Aug 28, 2018

lasote Aug 29, 2018

jgsogo Aug 29, 2018 •

edited

Loading

lasote Sep 6, 2018

jgsogo Aug 28, 2018

lasote Sep 6, 2018

jgsogo commented Aug 28, 2018 •

edited

Loading

memsharded left a comment

memsharded Aug 28, 2018

jgsogo commented Aug 29, 2018

memsharded Sep 4, 2018

jgsogo Sep 5, 2018

memsharded Sep 4, 2018

jgsogo Sep 5, 2018

lasote Sep 6, 2018

lasote Sep 6, 2018

		@@ -4,11 +4,23 @@
		from conans.model.version import Version


		# TODO: Move to some tools module

Add more meaninful error when parsing conanfile reference (closes #3204) #3410

Add more meaninful error when parsing conanfile reference (closes #3204) #3410

Conversation

jgsogo commented Aug 28, 2018 • edited by lasote Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo commented Aug 28, 2018 • edited Loading

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo commented Aug 28, 2018 •

edited by lasote

Loading

jgsogo Aug 29, 2018 •

edited

Loading

jgsogo commented Aug 28, 2018 •

edited

Loading