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 more meaninful error when parsing conanfile reference (closes #3204) #3410

Merged
merged 12 commits into from Sep 20, 2018

Conversation

Projects
None yet
3 participants
@jgsogo
Copy link
Member

commented Aug 28, 2018

  • Refer to the issue that supports this Pull Request. #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 jgsogo requested a review from lasote Aug 28, 2018

@ghost ghost assigned jgsogo Aug 28, 2018

@ghost ghost added the stage: review label Aug 28, 2018

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


# TODO: Move to some tools module

This comment has been minimized.

Copy link
@jgsogo

jgsogo Aug 28, 2018

Author Member

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)

This comment has been minimized.

Copy link
@memsharded

memsharded Aug 28, 2018

Contributor

Yes, use six tools, please.

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 comment has been minimized.

Copy link
@jgsogo

jgsogo Aug 28, 2018

Author Member

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

This comment has been minimized.

Copy link
@lasote

lasote Aug 29, 2018

Contributor

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

This comment has been minimized.

Copy link
@jgsogo

jgsogo Aug 29, 2018

Author Member

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.

This comment has been minimized.

Copy link
@lasote

lasote Sep 6, 2018

Contributor

I would say yes, we should raise it to 51

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

This comment has been minimized.

Copy link
@jgsogo

jgsogo Aug 28, 2018

Author Member

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

This comment has been minimized.

Copy link
@lasote

lasote Sep 6, 2018

Contributor

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

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Related stuff (maybe for a different PR):

  • validate_user (
    def validate_user(username):
    ) is used only in Username class (
    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
Copy link
Contributor

left a comment

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.

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


# TODO: Move to some tools module

This comment has been minimized.

Copy link
@memsharded

memsharded Aug 28, 2018

Contributor

Yes, use six tools, please.

@lasote lasote added this to the 1.8 milestone Aug 29, 2018

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2018

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

@jgsogo 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

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"))

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 4, 2018

Contributor

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.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Sep 5, 2018

Author Member

Ok, then I'll go for a different approach

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

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 4, 2018

Contributor

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

This comment has been minimized.

Copy link
@jgsogo

jgsogo Sep 5, 2018

Author Member

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).

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 comment has been minimized.

Copy link
@lasote

lasote Sep 6, 2018

Contributor

I would say yes, we should raise it to 51

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

This comment has been minimized.

Copy link
@lasote

lasote Sep 6, 2018

Contributor

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

@memsharded memsharded assigned lasote and unassigned jgsogo Sep 18, 2018

@lasote

lasote approved these changes Sep 20, 2018

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

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Sep 20, 2018

@jgsogo jgsogo deleted the jgsogo:issue/3204 branch Sep 26, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Add more meaninful error when parsing conanfile reference (closes con…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.