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

parse_version() should accept None and empty strings #10

Open
pombredanne opened this issue Oct 14, 2021 · 11 comments
Open

parse_version() should accept None and empty strings #10

pombredanne opened this issue Oct 14, 2021 · 11 comments
Labels

Comments

@pombredanne
Copy link
Collaborator

In https://github.com/nexB/univers/blob/63bd5aec16ec95b5b811ede638ac225f3ab1f6c6/src/univers/versions.py#L286
we should let parse_version() accept None and empty strings, otherwise the calling code has too much work to do.

Hritik14 added a commit to Hritik14/vulnerablecode that referenced this issue Oct 22, 2021
aboutcode-org/univers#10
- parse_version() should accept None and empty strings

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
@Hritik14
Copy link
Collaborator

@Hritik14
Copy link
Collaborator

To add,
Version(None) throws error

@Subhraneel77
Copy link

Adding version("") should work , as assigning "" to a variable to initialize an empty string in Python.
@pombredanne

Hritik14 added a commit to Hritik14/vulnerablecode that referenced this issue Jan 23, 2022
aboutcode-org/univers#10
- parse_version() should accept None and empty strings

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Hritik14 added a commit to Hritik14/vulnerablecode that referenced this issue Jan 23, 2022
aboutcode-org/univers#10
- parse_version() should accept None and empty strings

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Hritik14 added a commit to Hritik14/vulnerablecode that referenced this issue Jan 25, 2022
aboutcode-org/univers#10
- parse_version() should accept None and empty strings

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
@Hritik14
Copy link
Collaborator

Version(None) still throws an error.

>>> from univers.versions import Version
>>> Version(None)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-dece84ce917a> in <module>
----> 1 Version(None)

~/Contrib/univers/src/univers/versions.py in __init__(self, string, normalized_string, value)
      5     _inst_dict['normalized_string'] = normalized_string
      6     _inst_dict['value'] = value
----> 7     self.__attrs_post_init__()

~/Contrib/univers/src/univers/versions.py in __attrs_post_init__(self)
     59
     60     def __attrs_post_init__(self):
---> 61         normalized_string = self.normalize(self.string)
     62         if not self.is_valid(normalized_string):
     63             raise InvalidVersion(f"{self.string!r} is not a valid {self.__class__!r}")

~/Contrib/univers/src/univers/versions.py in normalize(cls, string)
     86         """
     87         # FIXME: Is removing spaces and strip v the right thing to do?
---> 88         return remove_spaces(string).rstrip("v")
     89
     90     @classmethod

~/Contrib/univers/src/univers/utils.py in remove_spaces(string)
      7
      8 def remove_spaces(string):
----> 9     return "".join(string.split())
     10
     11

AttributeError: 'NoneType' object has no attribute 'split'

@pombredanne
Copy link
Collaborator Author

Version(None) still throws an error.

IMHO that's a feature. When would a None version make sense?

@pombredanne
Copy link
Collaborator Author

Adding version("") should work , as assigning "" to a variable to initialize an empty string in Python. @pombredanne

Thanks, but None or empty would be the same... and I am not sure this would make any sense.

@Hritik14
Copy link
Collaborator

@pombredanne

When would a None version make sense?

Please see #33

@janniclas
Copy link

This causes problems in vulnerablecode when the importers/improvers get incomplete data as it is currently the case for some of them. This should be handled in the calling code in vulnerablecode ( PR aboutcode-org/vulnerablecode#1215 ).
However, I think we could also make a case for handling this in this lib, as this crash came rather unexpected to be honest.

@pombredanne
Copy link
Collaborator Author

@janniclas Thanks!
Now, what should we return in these cases? None?

@janniclas
Copy link

None is what I did in my workaround to fix the problem when I encountered it in vulnerablecode.
However, I'm no python dev so I don't know if this is the idiomatic solution for this, but it works fine in the calling code. I expect that None would also not break anything in any client code as the current version throws the value error for these cases which would directly crash the calling code if not handled.
TLDR; None would be finde for me.

@janniclas
Copy link

@pombredanne I just added a test case to the previously linked PR, maybe this is also helpful for you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants